Re: [Xen-devel] [PATCH] tools/kdd: silence gcc 8 warning a different way
On Tue, May 22, 2018 at 11:54:36AM +0100, Wei Liu wrote: > I have tried to revert 437e00fea04becc91c1b6bc1c0baa636b067a5cc and > reproduce the gcc 8.1 warning with Arch Linux's gcc 8.1 compiler. > Strangely it doesn't complain. > > I haven't got a Fedora 28 around (which Marek used). It will take some > time to set that up. I've tried it again and it still fails, even on gcc 8.1 on Fedora 28. Maybe that's about some extra CFLAGS in rpm build environment (there are quite a few of them, including -fcf-protection -fstack-clash-protection etc)? Anyway, I'll send updated patch in a moment (instead of the revert). -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? signature.asc Description: PGP signature ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] tools/kdd: silence gcc 8 warning a different way
I have tried to revert 437e00fea04becc91c1b6bc1c0baa636b067a5cc and reproduce the gcc 8.1 warning with Arch Linux's gcc 8.1 compiler. Strangely it doesn't complain. I haven't got a Fedora 28 around (which Marek used). It will take some time to set that up. Wei. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] tools/kdd: silence gcc 8 warning a different way
On Mon, Apr 16, 2018 at 02:43:32PM +0200, Marek Marczykowski wrote: > On Mon, Apr 16, 2018 at 06:00:37AM -0600, Jan Beulich wrote: > > >>> On 16.04.18 at 12:33, wrote: > > > On Thu, Apr 12, 2018 at 06:04:49AM -0600, Jan Beulich wrote: > > >> Older gcc doesn't like "#pragma GCC diagnostic" inside functions. > > >> > > >> Signed-off-by: Jan Beulich > > >> > > >> --- a/tools/debugger/kdd/kdd.c > > >> +++ b/tools/debugger/kdd/kdd.c > > >> @@ -695,10 +695,10 @@ static void kdd_handle_read_ctrl(kdd_sta > > >> KDD_LOG(s, "Request outside of known control space\n"); > > >> len = 0; > > >> } else { > > >> -#pragma GCC diagnostic push > > >> -#pragma GCC diagnostic ignored "-Warray-bounds" > > >> -memcpy(buf, ((uint8_t *)&ctrl.c32) + offset, len); > > >> -#pragma GCC diagnostic pop > > >> +/* Suppress bogus gcc 8 "out of bounds" warning. */ > > >> +const uint8_t *src; > > >> +asm ("" : "=g" (src) : "0" ((uint8_t *)&ctrl.c32 + offset)); > > >> +memcpy(buf, src, len); > > > > > > The code looks correct to me: > > > > > > Reviewed-by: Wei Liu > > > > > > This will hopefully also fix the issue Boris reported that some older > > > gcc (<4.6) doesn't support push and pop. > > > > > > This is the first time I see inline assembly is used to silence gcc. > > > ;-) > > > > And I'm not overly happy about it, but couldn't think of a better way > > without disabling said warning (or -Werror) altogether for the CU. If > > Ian's sketched out approach worked, I'd be quite happy to drop the > > patch here. > > What about changing offset type to uint32_t (or similar), which also > mute the warning? That should be fine, since that branch is handling 32 bit case. Tim in the other sub-thread also hinted something similar. Wei. > > -- > Best Regards, > Marek Marczykowski-Górecki > Invisible Things Lab > A: Because it messes up the order in which people normally read text. > Q: Why is top-posting such a bad thing? ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] tools/kdd: silence gcc 8 warning a different way
At 09:29 +0100 on 16 Apr (1523870989), Wei Liu wrote: > Cc Tim > On Thu, Apr 12, 2018 at 06:04:49AM -0600, Jan Beulich wrote: > > Older gcc doesn't like "#pragma GCC diagnostic" inside functions. > > > > Signed-off-by: Jan Beulich > > > > --- a/tools/debugger/kdd/kdd.c > > +++ b/tools/debugger/kdd/kdd.c > > @@ -695,10 +695,10 @@ static void kdd_handle_read_ctrl(kdd_sta > > KDD_LOG(s, "Request outside of known control space\n"); > > len = 0; > > } else { > > -#pragma GCC diagnostic push > > -#pragma GCC diagnostic ignored "-Warray-bounds" > > -memcpy(buf, ((uint8_t *)&ctrl.c32) + offset, len); > > -#pragma GCC diagnostic pop > > +/* Suppress bogus gcc 8 "out of bounds" warning. */ > > +const uint8_t *src; > > +asm ("" : "=g" (src) : "0" ((uint8_t *)&ctrl.c32 + offset)); That's terrifying! Does casting the offset to uint32_t not DTRT? Tim. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] tools/kdd: silence gcc 8 warning a different way
Jan Beulich writes ("Re: [PATCH] tools/kdd: silence gcc 8 warning a different way"): > And I'm not overly happy about it, but couldn't think of a better way > without disabling said warning (or -Werror) altogether for the CU. If > Ian's sketched out approach worked, I'd be quite happy to drop the > patch here. I did not sketch out an approach. Implicit, though, is the suggestion that the memcpy should be made conditional. Ian. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] tools/kdd: silence gcc 8 warning a different way
On Mon, Apr 16, 2018 at 06:00:37AM -0600, Jan Beulich wrote: > >>> On 16.04.18 at 12:33, wrote: > > On Thu, Apr 12, 2018 at 06:04:49AM -0600, Jan Beulich wrote: > >> Older gcc doesn't like "#pragma GCC diagnostic" inside functions. > >> > >> Signed-off-by: Jan Beulich > >> > >> --- a/tools/debugger/kdd/kdd.c > >> +++ b/tools/debugger/kdd/kdd.c > >> @@ -695,10 +695,10 @@ static void kdd_handle_read_ctrl(kdd_sta > >> KDD_LOG(s, "Request outside of known control space\n"); > >> len = 0; > >> } else { > >> -#pragma GCC diagnostic push > >> -#pragma GCC diagnostic ignored "-Warray-bounds" > >> -memcpy(buf, ((uint8_t *)&ctrl.c32) + offset, len); > >> -#pragma GCC diagnostic pop > >> +/* Suppress bogus gcc 8 "out of bounds" warning. */ > >> +const uint8_t *src; > >> +asm ("" : "=g" (src) : "0" ((uint8_t *)&ctrl.c32 + offset)); > >> +memcpy(buf, src, len); > > > > The code looks correct to me: > > > > Reviewed-by: Wei Liu > > > > This will hopefully also fix the issue Boris reported that some older > > gcc (<4.6) doesn't support push and pop. > > > > This is the first time I see inline assembly is used to silence gcc. > > ;-) > > And I'm not overly happy about it, but couldn't think of a better way > without disabling said warning (or -Werror) altogether for the CU. If > Ian's sketched out approach worked, I'd be quite happy to drop the > patch here. What about changing offset type to uint32_t (or similar), which also mute the warning? -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? signature.asc Description: PGP signature ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] tools/kdd: silence gcc 8 warning a different way
>>> On 16.04.18 at 12:33, wrote: > On Thu, Apr 12, 2018 at 06:04:49AM -0600, Jan Beulich wrote: >> Older gcc doesn't like "#pragma GCC diagnostic" inside functions. >> >> Signed-off-by: Jan Beulich >> >> --- a/tools/debugger/kdd/kdd.c >> +++ b/tools/debugger/kdd/kdd.c >> @@ -695,10 +695,10 @@ static void kdd_handle_read_ctrl(kdd_sta >> KDD_LOG(s, "Request outside of known control space\n"); >> len = 0; >> } else { >> -#pragma GCC diagnostic push >> -#pragma GCC diagnostic ignored "-Warray-bounds" >> -memcpy(buf, ((uint8_t *)&ctrl.c32) + offset, len); >> -#pragma GCC diagnostic pop >> +/* Suppress bogus gcc 8 "out of bounds" warning. */ >> +const uint8_t *src; >> +asm ("" : "=g" (src) : "0" ((uint8_t *)&ctrl.c32 + offset)); >> +memcpy(buf, src, len); > > The code looks correct to me: > > Reviewed-by: Wei Liu > > This will hopefully also fix the issue Boris reported that some older > gcc (<4.6) doesn't support push and pop. > > This is the first time I see inline assembly is used to silence gcc. > ;-) And I'm not overly happy about it, but couldn't think of a better way without disabling said warning (or -Werror) altogether for the CU. If Ian's sketched out approach worked, I'd be quite happy to drop the patch here. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] tools/kdd: silence gcc 8 warning a different way
Wei Liu writes ("Re: [PATCH] tools/kdd: silence gcc 8 warning a different way"): > On Thu, Apr 12, 2018 at 06:04:49AM -0600, Jan Beulich wrote: > > Older gcc doesn't like "#pragma GCC diagnostic" inside functions. Surely this commit message should refer to the previous commit ? I reviewed the previous code to check that the validation was correct: if (offset > sizeof ctrl.c32 || offset + len > sizeof ctrl.c32) { len = 0; } else { memcpy(buf, ((uint8_t *)&ctrl.c32) + offset, len); } I see two problems: 1. Adversarial optimissation hazard: The compiler may reason as follows: It is not legal to compute an out-of-bounds pointer. Doing so is UB. Therefore ((uint8_t *)&ctrl.c32) + offset (which is caclulated unconditionally) is within the stack object `ctrl'. Therefore offset <= sizeof(ctrl). 1a. The compiler can continue to reason: Suppose offset > sizeof(ctrl.c32) but offset + len <= sizeof(ctrl.c32). Because len is limited to 32-bit that can only happen if offset is large enough to wrap when len is added. But I know that offset <= 216. So this situation cannot exist. Therefore I can remove the check for offset and rely only on the check for offset + len. 1b. The compiler can continue to reason: So offset <= 216. I can therefore check that offset <= sizeof(ctrl.c32) using an instruction sequence that only looks at the bottom byte of offset (which on some architectures might be faster). 1c. If sizeof(ctrl.c32) ever becomes the same as sizeof(ctrl), the compiler can remove both checks entirely. 2. Style problem: Suppose len = (uint64_t)-1 offset = 1 The check passes, but the memcpy gets len = bazillion-1. In fact, len is a uint32_t so this is not possible but it is not possible to review these lines in isolation and see that they are correct. A future programmer might increase the size of len, introducing a bug. You should compile-time assert that len is 32-bit, somehow, right next to that check, IMO. > > +/* Suppress bogus gcc 8 "out of bounds" warning. */ > > +const uint8_t *src; > > +asm ("" : "=g" (src) : "0" ((uint8_t *)&ctrl.c32 + offset)); > > +memcpy(buf, src, len); > > The code looks correct to me: IMO the new code is very hard to follow. Are we really expecting every reader of this to know w3hat the asm does ? At the very least it should be accompanied with an explanation of what the asm does, for the benefit of readers who don't speak asm. I think that rather than introducing an asm, we should disable -Werror for known-buggy compilers. But it is possible that fixing the problems I identify above will make the warning go away anyway. Ian. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] tools/kdd: silence gcc 8 warning a different way
On Thu, Apr 12, 2018 at 06:04:49AM -0600, Jan Beulich wrote: > Older gcc doesn't like "#pragma GCC diagnostic" inside functions. > > Signed-off-by: Jan Beulich > > --- a/tools/debugger/kdd/kdd.c > +++ b/tools/debugger/kdd/kdd.c > @@ -695,10 +695,10 @@ static void kdd_handle_read_ctrl(kdd_sta > KDD_LOG(s, "Request outside of known control space\n"); > len = 0; > } else { > -#pragma GCC diagnostic push > -#pragma GCC diagnostic ignored "-Warray-bounds" > -memcpy(buf, ((uint8_t *)&ctrl.c32) + offset, len); > -#pragma GCC diagnostic pop > +/* Suppress bogus gcc 8 "out of bounds" warning. */ > +const uint8_t *src; > +asm ("" : "=g" (src) : "0" ((uint8_t *)&ctrl.c32 + offset)); > +memcpy(buf, src, len); The code looks correct to me: Reviewed-by: Wei Liu This will hopefully also fix the issue Boris reported that some older gcc (<4.6) doesn't support push and pop. This is the first time I see inline assembly is used to silence gcc. ;-) > } > } > > > > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] tools/kdd: silence gcc 8 warning a different way
Cc Tim On Thu, Apr 12, 2018 at 06:04:49AM -0600, Jan Beulich wrote: > Older gcc doesn't like "#pragma GCC diagnostic" inside functions. > > Signed-off-by: Jan Beulich > > --- a/tools/debugger/kdd/kdd.c > +++ b/tools/debugger/kdd/kdd.c > @@ -695,10 +695,10 @@ static void kdd_handle_read_ctrl(kdd_sta > KDD_LOG(s, "Request outside of known control space\n"); > len = 0; > } else { > -#pragma GCC diagnostic push > -#pragma GCC diagnostic ignored "-Warray-bounds" > -memcpy(buf, ((uint8_t *)&ctrl.c32) + offset, len); > -#pragma GCC diagnostic pop > +/* Suppress bogus gcc 8 "out of bounds" warning. */ > +const uint8_t *src; > +asm ("" : "=g" (src) : "0" ((uint8_t *)&ctrl.c32 + offset)); > +memcpy(buf, src, len); > } > } > > > > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel