[PATCH] tools/kdd: silence gcc 8 warning a different way

[PATCH] tools/kdd: silence gcc 8 warning a different way

[Jan Beulich]

Older gcc doesn't like "#pragma GCC diagnostic" inside functions.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- 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

Re: [PATCH] tools/kdd: silence gcc 8 warning a different way

[Wei Liu]

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 <jbeulich@suse.com>
> 
> --- 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

Re: [PATCH] tools/kdd: silence gcc 8 warning a different way

[Wei Liu]

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 <jbeulich@suse.com>
> 
> --- 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 <wei.liu2@citrix.com>

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: [PATCH] tools/kdd: silence gcc 8 warning a different way

[Ian Jackson]

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: [PATCH] tools/kdd: silence gcc 8 warning a different way

[Jan Beulich]

>>> On 16.04.18 at 12:33, <wei.liu2@citrix.com> 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 <jbeulich@suse.com>
>> 
>> --- 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 <wei.liu2@citrix.com>
> 
> 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: [PATCH] tools/kdd: silence gcc 8 warning a different way

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 1843 bytes --]

On Mon, Apr 16, 2018 at 06:00:37AM -0600, Jan Beulich wrote:
> >>> On 16.04.18 at 12:33, <wei.liu2@citrix.com> 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 <jbeulich@suse.com>
> >> 
> >> --- 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 <wei.liu2@citrix.com>
> > 
> > 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?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] tools/kdd: silence gcc 8 warning a different way

[Ian Jackson]

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: [PATCH] tools/kdd: silence gcc 8 warning a different way

[Tim Deegan]

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 <jbeulich@suse.com>
> > 
> > --- 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: [PATCH] tools/kdd: silence gcc 8 warning a different way

[Wei Liu]

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, <wei.liu2@citrix.com> 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 <jbeulich@suse.com>
> > >> 
> > >> --- 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 <wei.liu2@citrix.com>
> > > 
> > > 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: [PATCH] tools/kdd: silence gcc 8 warning a different way

[Wei Liu]

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: [PATCH] tools/kdd: silence gcc 8 warning a different way

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 818 bytes --]

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?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel