| Message ID | 1481479437-30537-2-git-send-email-notasas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11 December 2016 at 18:03, Grazvydas Ignotas <notasas@gmail.com> wrote: > Just tell the compiler that drm_event will alias the char buffer, > so that it has no excuse to warn or generate bad code. > Afacit this patch [1] from Thierry should correctly address the issue, correct ? I've been meaning to parse through his branches [2] for fixes/others but never got to it. Are you interested in giving it a go ? Thanks Emil [1] https://cgit.freedesktop.org/~tagr/drm/commit/?h=staging/fixes&id=4ebf7b59e69e43cc3810141dff8e3ef3535f03b2 [2] https://cgit.freedesktop.org/~tagr/drm/
On Mon, Dec 12, 2016 at 3:59 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > On 11 December 2016 at 18:03, Grazvydas Ignotas <notasas@gmail.com> wrote: >> Just tell the compiler that drm_event will alias the char buffer, >> so that it has no excuse to warn or generate bad code. >> > Afacit this patch [1] from Thierry should correctly address the issue, correct ? From what I've read, gcc's "strict aliasing" means that it's illegal to access the same memory location using pointers of different types, with only one exception - accessing an object of any type through a char pointer. What is done here is the opposite - char array is read as a struct, so according to that it's still wrong. That said I haven't seen any compiler causing problems in this case, and Thierry's solution indeed silences the warning, so I guess you can take his patch if you prefer. GraÅžvydas
On 12 December 2016 at 23:28, Grazvydas Ignotas <notasas@gmail.com> wrote: > On Mon, Dec 12, 2016 at 3:59 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >> On 11 December 2016 at 18:03, Grazvydas Ignotas <notasas@gmail.com> wrote: >>> Just tell the compiler that drm_event will alias the char buffer, >>> so that it has no excuse to warn or generate bad code. >>> >> Afacit this patch [1] from Thierry should correctly address the issue, correct ? > > From what I've read, gcc's "strict aliasing" means that it's illegal > to access the same memory location using pointers of different types, > with only one exception - accessing an object of any type through a > char pointer. What is done here is the opposite - char array is read > as a struct, so according to that it's still wrong. > > That said I haven't seen any compiler causing problems in this case, > and Thierry's solution indeed silences the warning, so I guess you can > take his patch if you prefer. > I've seen a lot of noise on the topic of strict aliasing yet never had the time to investigate [too much]. There's some argument(s) that with type based (strict) aliasing the GCC [used to at least] make incorrect assumptions reordering code (stores). Latter of which causing issues when combined with optimisations. That aside: afaict the warning there is triggered since we have "& some_char" on rhs, rather than "some_char_or_void_star". With the latter of which explicitly allowed on the topic. The strange this is that fleshing a [identical] small example triggers no warning, regardless of the level (1,2,3) $ gcc -Wall -Wextra -Wstrict-aliasing=3 ss.c Barring any objections I'm leaning towards Thierry's patch. Thierry did you see any side-effect with [1] or you simply did not have time to double-check/investigate if the problem is truly fixed. Just double-checking. Thanks ! Emil [1] https://cgit.freedesktop.org/~tagr/drm/commit/?h=staging/fixes&id=4ebf7b59e69e43cc3810141dff8e3ef3535f03b2
On Wed, Dec 14, 2016 at 03:46:03PM +0000, Emil Velikov wrote: > On 12 December 2016 at 23:28, Grazvydas Ignotas <notasas@gmail.com> wrote: > > On Mon, Dec 12, 2016 at 3:59 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > >> On 11 December 2016 at 18:03, Grazvydas Ignotas <notasas@gmail.com> wrote: > >>> Just tell the compiler that drm_event will alias the char buffer, > >>> so that it has no excuse to warn or generate bad code. > >>> > >> Afacit this patch [1] from Thierry should correctly address the issue, correct ? > > > > From what I've read, gcc's "strict aliasing" means that it's illegal > > to access the same memory location using pointers of different types, > > with only one exception - accessing an object of any type through a > > char pointer. What is done here is the opposite - char array is read > > as a struct, so according to that it's still wrong. > > > > That said I haven't seen any compiler causing problems in this case, > > and Thierry's solution indeed silences the warning, so I guess you can > > take his patch if you prefer. > > > I've seen a lot of noise on the topic of strict aliasing yet never had > the time to investigate [too much]. > There's some argument(s) that with type based (strict) aliasing the > GCC [used to at least] make incorrect assumptions reordering code > (stores). Latter of which causing issues when combined with > optimisations. > > That aside: afaict the warning there is triggered since we have "& > some_char" on rhs, rather than "some_char_or_void_star". With the > latter of which explicitly allowed on the topic. > The strange this is that fleshing a [identical] small example triggers > no warning, regardless of the level (1,2,3) > $ gcc -Wall -Wextra -Wstrict-aliasing=3 ss.c > > Barring any objections I'm leaning towards Thierry's patch. > > Thierry did you see any side-effect with [1] or you simply did not > have time to double-check/investigate if the problem is truly fixed. > Just double-checking. I haven't done any exhaustive or focussed testing, but I've been running the libdrm from my tree on a couple of systems and never seen any issues with drmHandleEvent(). My understanding of the problem is that aliasing is only a problem if an optimizing path would discard accesses to the aliased memory. I don't think the existing code would cause any issues because we never mix any accesses to buffer and e. I think therefore the warning is a false positive, though gcc might not be looking thoroughly enough to determine that it's harmless. Perhaps a better way to solve this would be to not rely on any casting whatsoever. We could for example add a union that contains all possible events and read that in one at a time. However, drivers can provide their own events, so the union isn't really an option. That said, since we limit ourselves to a 1 KiB buffer any events that are larger than 1 KiB (there probably aren't any) would be breaking libdrm because drmHandleEvent() wouldn't be able to read the event and therefore it won't be removed from the queue. Interestingly the kerneldoc for drm_read() says that the maximum event space is currently 4 KiB, so I guess libdrm could use an update to use that maximum. However, upon further inspection I don't see any code in the kernel that enforces this limit... Let me see if I can untangle that with some patches... Thierry
diff --git a/Android.mk b/Android.mk index 2aa2bc9..ae7c3fe 100644 --- a/Android.mk +++ b/Android.mk @@ -42,6 +42,7 @@ LOCAL_C_INCLUDES := \ LOCAL_CFLAGS := \ -DHAVE_VISIBILITY=1 \ + -DHAVE_MAY_ALIAS=1 \ -DHAVE_LIBDRM_ATOMIC_PRIMITIVES=1 include $(BUILD_STATIC_LIBRARY) diff --git a/configure.ac b/configure.ac index e0597c3..2868036 100644 --- a/configure.ac +++ b/configure.ac @@ -507,6 +507,15 @@ if test "x$HAVE_ATTRIBUTE_VISIBILITY" = xyes; then AC_DEFINE(HAVE_VISIBILITY, 1, [Compiler supports __attribute__(("hidden"))]) fi +AC_MSG_CHECKING([whether $CC supports __attribute__((__may_alias__))]) +AC_LINK_IFELSE([AC_LANG_PROGRAM([ + struct foo_may_alias { int i; } __attribute__((__may_alias__)) foo; +])], HAVE_ATTRIBUTE_MAY_ALIAS="yes"; AC_MSG_RESULT([yes]), AC_MSG_RESULT([no])); + +if test "x$HAVE_ATTRIBUTE_MAY_ALIAS" = xyes; then + AC_DEFINE(HAVE_MAY_ALIAS, 1, [Compiler supports __attribute__((__may_alias__))]) +fi + AC_SUBST(WARN_CFLAGS) AC_CONFIG_FILES([ Makefile diff --git a/libdrm_macros.h b/libdrm_macros.h index 639d090..eea47ee 100644 --- a/libdrm_macros.h +++ b/libdrm_macros.h @@ -29,6 +29,12 @@ # define drm_private #endif +#if defined(HAVE_MAY_ALIAS) +# define drm_may_alias __attribute__((__may_alias__)) +#else +# define drm_may_alias +#endif + /** * Static (compile-time) assertion. diff --git a/xf86drmMode.c b/xf86drmMode.c index fb22f68..a6bf5f7 100644 --- a/xf86drmMode.c +++ b/xf86drmMode.c @@ -52,6 +52,7 @@ #include <stdio.h> #include <stdbool.h> +#include "libdrm_macros.h" #include "xf86drmMode.h" #include "xf86drm.h" #include <drm.h> @@ -885,9 +886,11 @@ int drmModeCrtcSetGamma(int fd, uint32_t crtc_id, uint32_t size, int drmHandleEvent(int fd, drmEventContextPtr evctx) { + struct drm_event_aliased { + struct drm_event e; + } drm_may_alias *e; char buffer[1024]; int len, i; - struct drm_event *e; struct drm_event_vblank *vblank; /* The DRM read semantics guarantees that we always get only @@ -901,8 +904,8 @@ int drmHandleEvent(int fd, drmEventContextPtr evctx) i = 0; while (i < len) { - e = (struct drm_event *) &buffer[i]; - switch (e->type) { + e = (struct drm_event_aliased *) &buffer[i]; + switch (e->e.type) { case DRM_EVENT_VBLANK: if (evctx->version < 1 || evctx->vblank_handler == NULL) @@ -928,7 +931,7 @@ int drmHandleEvent(int fd, drmEventContextPtr evctx) default: break; } - i += e->length; + i += e->e.length; } return 0;
Just tell the compiler that drm_event will alias the char buffer, so that it has no excuse to warn or generate bad code. Signed-off-by: Grazvydas Ignotas <notasas@gmail.com> --- Android.mk | 1 + configure.ac | 9 +++++++++ libdrm_macros.h | 6 ++++++ xf86drmMode.c | 11 +++++++---- 4 files changed, 23 insertions(+), 4 deletions(-)