On Wed, Dec 14, 2016 at 03:46:03PM +0000, Emil Velikov wrote: > On 12 December 2016 at 23:28, Grazvydas Ignotas <notasas@xxxxxxxxx> wrote: > > On Mon, Dec 12, 2016 at 3:59 PM, Emil Velikov <emil.l.velikov@xxxxxxxxx> wrote: > >> On 11 December 2016 at 18:03, Grazvydas Ignotas <notasas@xxxxxxxxx> 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
