[PATCH libdrm] xf86drm: fix aliasing violation

[Thierry Reding]

On Wed, Dec 14, 2016 at 03:46:03PM +0000, Emil Velikov wrote:
> On 12 December 2016 at 23:28, Grazvydas Ignotas <notasas at gmail.com> wrote:
> > On Mon, Dec 12, 2016 at 3:59 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> >> On 11 December 2016 at 18:03, Grazvydas Ignotas <notasas at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161214/ea6682db/attachment.sig>