From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH libdrm] xf86drm: fix aliasing violation Date: Wed, 14 Dec 2016 18:11:39 +0100 Message-ID: <20161214171139.GB30280@ulmo.ba.sec> References: <1481479437-30537-2-git-send-email-notasas@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2068226984==" Return-path: Received: from mail-wj0-x241.google.com (mail-wj0-x241.google.com [IPv6:2a00:1450:400c:c01::241]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2BF8589A20 for ; Wed, 14 Dec 2016 17:11:43 +0000 (UTC) Received: by mail-wj0-x241.google.com with SMTP id he10so5912613wjc.2 for ; Wed, 14 Dec 2016 09:11:43 -0800 (PST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Emil Velikov Cc: ML dri-devel List-Id: dri-devel@lists.freedesktop.org --===============2068226984== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="GID0FwUMdk1T2AWN" Content-Disposition: inline --GID0FwUMdk1T2AWN Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Dec 14, 2016 at 03:46:03PM +0000, Emil Velikov wrote: > On 12 December 2016 at 23:28, Grazvydas Ignotas wrote: > > On Mon, Dec 12, 2016 at 3:59 PM, Emil Velikov wrote: > >> On 11 December 2016 at 18:03, Grazvydas Ignotas wr= ote: > >>> 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. >=20 > 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=3D3 ss.c >=20 > Barring any objections I'm leaning towards Thierry's patch. >=20 > 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 --GID0FwUMdk1T2AWN Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAlhRfUgACgkQ3SOs138+ s6FVVw//XJi/nDWOSHz4iC9TYmdOtEqa74v1eTrAdok8lOOK1GZR5X+o5UPUiDnQ j4F37ztriaOenpBVPgu6jkTt6tDIfue6S1hOIZwolOdgx1k4uwSKN4c3NvrBSZvg i3c1TrMBBLQ+p87vu34D3KmzQiffWA6Mh8JhM8DrOM00wk1HvlEQyb8lRa3+KKnk F7xZDDMmhmYf7d/vyUKcYnPN5ZS8pDp242OPnntr8NGtku68/8qWGNQT9a8j7Upn p1kyY+k0uAUrbtN8kPCxsVCSmpwSeEfgyrwY93wDkQfQYVwN2AAuzcQ7h6OwonbY BxxuwaK+jVnFXS2UkufgFo01idtEf2bW8sCM07iR94aJekpU3EuULqoz31FsZrz3 hz+TAEBaElq42NJUyus9nI5D17uh37MOBYGPW4bg34BmeRvI8QuuPFNzYErNtK8Q uL+zFx5vXH7+VImTpW+uf6T5W2FB8IUZcuVYW88wSRMM9rNhvnNgLyVpiJehkHMT qeTs23C2CBFacvvqASxlcJk8drzBnEaPDn8HbpXTyapt6xDkw3iE2igFcTHo1VI4 8tEzU33xBDFyNwqJw/dkSowsLCj+EdYm7bR8CEeJ9XHLex/1P3RbJg+SGjgqyzVQ BgRHtjqI+sPKstfXxSqnhKe3YARD5ZUWyYy94jv+vtFxltwUGs8= =6wOY -----END PGP SIGNATURE----- --GID0FwUMdk1T2AWN-- --===============2068226984== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============2068226984==--