Re: [PATCH] Hexagon (target/hexagon) Fix shadow variable when idef-parser is off

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 30/11/23 21:39, Brian Cain wrote:
> 
> 
>> -----Original Message-----
>> From: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Sent: Thursday, November 30, 2023 2:17 PM
>> To: Taylor Simpson <ltaylorsimpson@gmail.com>; qemu-devel@nongnu.org
>> Cc: Brian Cain <bcain@quicinc.com>; Matheus Bernardino (QUIC)
>> <quic_mathbern@quicinc.com>; Sid Manning <sidneym@quicinc.com>; Marco
>> Liebel (QUIC) <quic_mliebel@quicinc.com>; richard.henderson@linaro.org;
>> ale@rev.ng; anjo@rev.ng
>> Subject: Re: [PATCH] Hexagon (target/hexagon) Fix shadow variable when idef-
>> parser is off
>>
>> WARNING: This email originated from outside of Qualcomm. Please be wary of
>> any links or attachments, and do not enable macros.
>>
>> On 30/11/23 19:39, Taylor Simpson wrote:
>>> Adding -Werror=shadow=compatible-local causes Hexagon not to build
>>> when idef-parser is off.  The "label" variable in CHECK_NOSHUF_PRED
>>> shadows a variable in the surrounding code.
>>>
>>> Signed-off-by: Taylor Simpson <ltaylorsimpson@gmail.com>
>>> ---
>>>    target/hexagon/macros.h | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
>>> index 9a51b5709b..f99390e2a8 100644
>>> --- a/target/hexagon/macros.h
>>> +++ b/target/hexagon/macros.h
>>> @@ -93,13 +93,13 @@
>>>
>>>    #define CHECK_NOSHUF_PRED(GET_EA, SIZE, PRED) \
>>>        do { \
>>> -        TCGLabel *label = gen_new_label(); \
>>> -        tcg_gen_brcondi_tl(TCG_COND_EQ, PRED, 0, label); \
>>> +        TCGLabel *noshuf_label = gen_new_label(); \
>>> +        tcg_gen_brcondi_tl(TCG_COND_EQ, PRED, 0, noshuf_label); \
>>
>> Fragile, but sufficient.
> 
> The fragility here refers to the fact that CHECK_NOSHUF_PRED() macro could show up in other contexts and then could shadow those?

Yes.

> We could change the macro to a function or expand the macro to take a label declared outside.  Would that be preferred?  Or are there other suggestions?

Nah, this is good enough, no need to over-engineer IMHO
(I just wanted to remark this could still bit us in the
future, and -Werror will fire).

Regards,

Phil.