Re: [Xen-devel] [PATCH 7/7] tools/kdd: mute spurious gcc warning

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Apr 06, 2018 at 09:56:05AM -0400, Boris Ostrovsky wrote:
> On 04/06/2018 09:41 AM, Wei Liu wrote:
> > On Fri, Apr 06, 2018 at 09:39:50AM -0400, Boris Ostrovsky wrote:
> >> On 04/06/2018 09:07 AM, Wei Liu wrote:
> >>> On Fri, Apr 06, 2018 at 08:39:53AM -0400, Boris Ostrovsky wrote:
> >>>> On 04/04/2018 09:50 PM, Marek Marczykowski-Górecki wrote:
> >>>>> gcc-8 complains:
> >>>>>
> >>>>>     kdd.c:698:13: error: 'memcpy' offset [-204, -717] is out of the 
> >>>>> bounds [0, 216] of object 'ctrl' with type 'kdd_ctrl' {aka 'union 
> >>>>> <anonymous>'} [-Werror=array-bounds]
> >>>>>                  memcpy(buf, ((uint8_t *)&ctrl.c32) + offset, len);
> >>>>>                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>>>>     kdd.c: In function 'kdd_select_callback':
> >>>>>     kdd.c:642:14: note: 'ctrl' declared here
> >>>>>          kdd_ctrl ctrl;
> >>>>>                   ^~~~
> >>>>>
> >>>>> But this is impossible - 'offset' is unsigned and correctly validated
> >>>>> few lines before.
> >>>>>
> >>>>> Signed-off-by: Marek Marczykowski-Górecki 
> >>>>> <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> >>>>> ---
> >>>>>  tools/debugger/kdd/kdd.c | 3 +++
> >>>>>  1 file changed, 3 insertions(+)
> >>>>>
> >>>>> diff --git a/tools/debugger/kdd/kdd.c b/tools/debugger/kdd/kdd.c
> >>>>> index 1bd5dd5..61d769e 100644
> >>>>> --- a/tools/debugger/kdd/kdd.c
> >>>>> +++ b/tools/debugger/kdd/kdd.c
> >>>>> @@ -695,7 +695,10 @@ static void kdd_handle_read_ctrl(kdd_state *s)
> >>>>>              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
> >>>>>          }
> >>>>>      }
> >>>>>  
> >>>> Breaks 32-bit build, at least with my (ancient, gcc version 4.4.5
> >>>> 20101112 (Red Hat 4.4.5-2) (GCC)) compiler:
> >>>>
> >>>>
> >>>>
> >>>> kdd.c: In function ‘kdd_handle_read_ctrl’:
> >>>> kdd.c:698: error: #pragma GCC diagnostic not allowed inside functions
> >>>> kdd.c:699: error: #pragma GCC diagnostic not allowed inside functions
> >>>> kdd.c:701: error: #pragma GCC diagnostic not allowed inside functions
> >>>> make[5]: *** [kdd.o] Error 1
> >>>>
> >>> Does moving the relevant #pragma's outside of the function fix it?
> >> The additional problem with these pragmas is that apparently push/pop
> >> have been introduced in gcc 4.6.0:
> >>
> >> https://gcc.gnu.org/onlinedocs/gcc-4.6.0/gcc/Diagnostic-Pragmas.html#Diagnostic-Pragmas
> >>
> >> If you change release number to a lower one (e.g. 4.5.4) you won't see 
> >> them.
> >>
> >> So I can move "diagnostic ignored" from inside the function and that
> >> will clear the "GCC diagnostic not allowed inside functions" error. But
> >> then push/pop are not recognized:
> >>
> >> cc1: warnings being treated as errors
> >> kdd.c:639: error: expected [error|warning|ignored] after ‘#pragma GCC
> >> diagnostic’
> >> kdd.c:714: error: expected [error|warning|ignored] after ‘#pragma GCC
> >> diagnostic’
> >>
> >> (Interestingly, my 64-bit build completed without issues)
> > Hmm... this is messy.
> >
> > If you have information about which version does what we can try to
> > enclose the #pragma's with #if __GCC__.
> 
> 
> Can we instead pre-compute the pointer to pacify the compiler? I haven't
> seen the original error so I can't test it, but something like

Nope, it doesn't help. But adding "if (offset > 0)" before that "+=
offset" does... 
For me it looks like a gcc bug. Not sure how to deal with this. Enclose
#pragma with #if __GNUC__ >= 8 ?

> diff --git a/tools/debugger/kdd/kdd.c b/tools/debugger/kdd/kdd.c
> index 61d769e..1b048ac 100644
> --- a/tools/debugger/kdd/kdd.c
> +++ b/tools/debugger/kdd/kdd.c
> @@ -688,6 +688,7 @@ static void kdd_handle_read_ctrl(kdd_state *s)
>      } else {
>          /* 32-bit control-register space starts at 0x[2]cc, for 84 bytes */
>          uint64_t offset = addr;
> +        void *ptr = &ctrl.c32;
>          if (offset > 0x200)
>              offset -= 0x200;
>          offset -= 0xcc;
> @@ -695,10 +696,8 @@ static void kdd_handle_read_ctrl(kdd_state *s)
>              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
> +            ptr += offset;
> +            memcpy(buf, ptr, len);
>          }
>      }
>  
> -boris

-- 
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: signature.asc
Description: PGP signature

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