| Message ID | 20231130183955.54314-1-ltaylorsimpson@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Hexagon (target/hexagon) Fix shadow variable when idef-parser is off | expand |
> -----Original Message----- > From: Taylor Simpson <ltaylorsimpson@gmail.com> > Sent: Thursday, November 30, 2023 12:40 PM > To: 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; > philmd@linaro.org; ale@rev.ng; anjo@rev.ng; ltaylorsimpson@gmail.com > Subject: [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. > > 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); \ > GET_EA; \ > if (insn->slot == 0 && ctx->pkt->pkt_has_store_s1) { \ > probe_noshuf_load(EA, SIZE, ctx->mem_idx); \ > } \ > - gen_set_label(label); \ > + gen_set_label(noshuf_label); \ > if (insn->slot == 0 && ctx->pkt->pkt_has_store_s1) { \ > process_store(ctx, 1); \ > } \ > -- > 2.34.1 Reviewed-by: Brian Cain <bcain@quicinc.com>
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. Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> -----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? 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? -Brian
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.
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); \ GET_EA; \ if (insn->slot == 0 && ctx->pkt->pkt_has_store_s1) { \ probe_noshuf_load(EA, SIZE, ctx->mem_idx); \ } \ - gen_set_label(label); \ + gen_set_label(noshuf_label); \ if (insn->slot == 0 && ctx->pkt->pkt_has_store_s1) { \ process_store(ctx, 1); \ } \
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(-)