[v3] RISC-V: Fix build with GCC-8
Commit Message
From: Mark Zhuang <mark.zhuang@spacemit.com>
GCC 8.5.0 rejects a declaration right after a case label.
Add braces to fix it.
---
gas/config/tc-riscv.c | 50 +++++++++++++++++++++----------------------
opcodes/riscv-dis.c | 16 ++++++++------
2 files changed, 35 insertions(+), 31 deletions(-)
Comments
On Tue, 23 Jun 2026, Mark Zhuang wrote:
> From: Mark Zhuang <mark.zhuang@spacemit.com>
>
> GCC 8.5.0 rejects a declaration right after a case label.
> Add braces to fix it.
I think it would be more useful to the reader if the commit referred to
the relevant C language standard and minimum compiler version as given by
Andreas: <https://inbox.sourceware.org/binutils/mvmtsqtzgve.fsf@suse.de/>.
FWIW it broke the build with GCC 10.2.1 here (production box, system
compiler).
Maciej
On 25.06.2026 23:43, Maciej W. Rozycki wrote:
> On Tue, 23 Jun 2026, Mark Zhuang wrote:
>
>> From: Mark Zhuang <mark.zhuang@spacemit.com>
>>
>> GCC 8.5.0 rejects a declaration right after a case label.
>> Add braces to fix it.
>
> I think it would be more useful to the reader if the commit referred to
> the relevant C language standard and minimum compiler version as given by
> Andreas: <https://inbox.sourceware.org/binutils/mvmtsqtzgve.fsf@suse.de/>.
To be honest, I think relatively basic standard C aspects should be known
to people, and "rejects a declaration right after a case label" is enough
of a pointer imo.
> FWIW it broke the build with GCC 10.2.1 here (production box, system
> compiler).
At the risk of coming across blunt, but: You complain about too little
detail above, and then you report this issue without providing any detail?
Jan
On 23.06.2026 15:35, Mark Zhuang wrote:
> From: Mark Zhuang <mark.zhuang@spacemit.com>
>
> GCC 8.5.0 rejects a declaration right after a case label.
> Add braces to fix it.
While I said I'd take care of getting this in, now I really can't seeing that
Maciej reported this breaks gcc10 (in a way unknown to me, as of yet).
As this made me look at the patch again (just to see if I can spot anything,
which I couldn't), ...
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -1791,8 +1791,6 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length)
> }
> break;
> case 'p': /* Vendor-specific (SpacemiT) operands. */
> - size_t n;
> - size_t s;
> switch (*++oparg)
> {
> case 'V':
> @@ -1815,13 +1813,15 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length)
> break;
> case 'u': /* Integer immediate, 'XpuN@S' ...
> N-bit unsigned immediate at bit S. */
> - n = strtol (oparg + 1, (char **)&oparg, 10);
> - if (*oparg != '@')
> - goto unknown_validate_operand;
> - s = strtol (oparg + 1, (char **)&oparg, 10);
> - oparg--;
> - USE_IMM (n, s);
> - break;
> + {
> + size_t n = strtol (oparg + 1, (char **)&oparg, 10);
> + if (*oparg != '@')
> + goto unknown_validate_operand;
> + size_t s = strtol (oparg + 1, (char **)&oparg, 10);
> + oparg--;
> + USE_IMM (n, s);
> + break;
> + }
... I think the "break" here and ...
> @@ -4309,8 +4309,6 @@ riscv_ip (char *str, struct riscv_cl_insn *ip, expressionS *imm_expr,
> break;
>
> case 'p': /* Vendor-specific (SpacemiT) operands. */
> - size_t n;
> - size_t s;
> switch (*++oparg)
> {
> case 'V':
> @@ -4358,20 +4356,22 @@ riscv_ip (char *str, struct riscv_cl_insn *ip, expressionS *imm_expr,
> break;
> case 'u': /* Integer immediate, 'XpuN@S' ...
> N-bit unsigned immediate at bit S. */
> - n = strtol (oparg + 1, (char **)&oparg, 10);
> - if (*oparg != '@')
> - goto unknown_riscv_ip_operand;
> - s = strtol (oparg + 1, (char **)&oparg, 10);
> - oparg--;
> - my_getExpression (imm_expr, asarg, force_reloc);
> - check_absolute_expr (ip, imm_expr, false);
> - if (!VALIDATE_U_IMM (imm_expr->X_add_number, n))
> - as_bad (_("improper immediate value (%"PRIu64")"),
> - imm_expr->X_add_number);
> - INSERT_IMM (n, s, *ip, imm_expr->X_add_number);
> - imm_expr->X_op = O_absent;
> - asarg = expr_parse_end;
> - continue;
> + {
> + size_t n = strtol (oparg + 1, (char **)&oparg, 10);
> + if (*oparg != '@')
> + goto unknown_riscv_ip_operand;
> + size_t s = strtol (oparg + 1, (char **)&oparg, 10);
> + oparg--;
> + my_getExpression (imm_expr, asarg, force_reloc);
> + check_absolute_expr (ip, imm_expr, false);
> + if (!VALIDATE_U_IMM (imm_expr->X_add_number, n))
> + as_bad (_("improper immediate value (%"PRIu64")"),
> + imm_expr->X_add_number);
> + INSERT_IMM (n, s, *ip, imm_expr->X_add_number);
> + imm_expr->X_op = O_absent;
> + asarg = expr_parse_end;
> + continue;
> + }
... the "continue" here would be nice to keep in their original positions,
just like ...
> --- a/opcodes/riscv-dis.c
> +++ b/opcodes/riscv-dis.c
> @@ -924,14 +924,18 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info
> switch (*++oparg)
> {
> case 'd':
> - unsigned vd = EXTRACT_OPERAND (SPACEMIT_IME_VD, l) * 2;
> - print (info->stream, dis_style_register, "%s",
> - riscv_vecr_names_numeric[vd]);
> + {
> + unsigned vd = EXTRACT_OPERAND (SPACEMIT_IME_VD, l) * 2;
> + print (info->stream, dis_style_register, "%s",
> + riscv_vecr_names_numeric[vd]);
> + }
> break;
> case 's':
> - unsigned vs = EXTRACT_OPERAND (SPACEMIT_IME_VS1, l) * 2;
> - print (info->stream, dis_style_register, "%s",
> - riscv_vecr_names_numeric[vs]);
> + {
> + unsigned vs = EXTRACT_OPERAND (SPACEMIT_IME_VS1, l) * 2;
> + print (info->stream, dis_style_register, "%s",
> + riscv_vecr_names_numeric[vs]);
> + }
> break;
... you do here. Once again, I'm happy to adjust while committing, just that
first we need to understand and address the gcc10 issue. Maciej, please
provide details.
Jan
On Fri, 26 Jun 2026, Jan Beulich wrote:
> > I think it would be more useful to the reader if the commit referred to
> > the relevant C language standard and minimum compiler version as given by
> > Andreas: <https://inbox.sourceware.org/binutils/mvmtsqtzgve.fsf@suse.de/>.
>
> To be honest, I think relatively basic standard C aspects should be known
> to people, and "rejects a declaration right after a case label" is enough
> of a pointer imo.
I don't expect everyone to track or remember all the individual features
as they are added to standard revisions or compiler implementations, and
details provided by Andreas are useful in determining the prerequsites.
Indeed the breakage caused in the first place serves as a proof: you'd
imply the code writer as well the approver should have known this feature
isn't there in every compiler version supported for building binutils, and
yet they missed it.
Why do you think it would be preferable to require everyone to chase
every time something that has been already determined?
> > FWIW it broke the build with GCC 10.2.1 here (production box, system
> > compiler).
>
> At the risk of coming across blunt, but: You complain about too little
> detail above, and then you report this issue without providing any detail?
Why would I need to repeat the same exact details as with the original
report? This is just to confirm that the same issue triggers for versions
other than 8.5.0 (just as Andreas has indicated). I reckon Andrew Burgess
also reported breakage with his environment.
Maciej
On Fri, 26 Jun 2026, Jan Beulich wrote:
> > --- a/opcodes/riscv-dis.c
> > +++ b/opcodes/riscv-dis.c
> > @@ -924,14 +924,18 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info
> > switch (*++oparg)
> > {
> > case 'd':
> > - unsigned vd = EXTRACT_OPERAND (SPACEMIT_IME_VD, l) * 2;
> > - print (info->stream, dis_style_register, "%s",
> > - riscv_vecr_names_numeric[vd]);
> > + {
> > + unsigned vd = EXTRACT_OPERAND (SPACEMIT_IME_VD, l) * 2;
> > + print (info->stream, dis_style_register, "%s",
> > + riscv_vecr_names_numeric[vd]);
> > + }
> > break;
> > case 's':
> > - unsigned vs = EXTRACT_OPERAND (SPACEMIT_IME_VS1, l) * 2;
> > - print (info->stream, dis_style_register, "%s",
> > - riscv_vecr_names_numeric[vs]);
> > + {
> > + unsigned vs = EXTRACT_OPERAND (SPACEMIT_IME_VS1, l) * 2;
> > + print (info->stream, dis_style_register, "%s",
> > + riscv_vecr_names_numeric[vs]);
> > + }
> > break;
>
> ... you do here. Once again, I'm happy to adjust while committing, just that
> first we need to understand and address the gcc10 issue. Maciej, please
> provide details.
The original report has them.
Maciej
On 26.06.2026 13:02, Maciej W. Rozycki wrote:
> On Fri, 26 Jun 2026, Jan Beulich wrote:
>
>>> I think it would be more useful to the reader if the commit referred to
>>> the relevant C language standard and minimum compiler version as given by
>>> Andreas: <https://inbox.sourceware.org/binutils/mvmtsqtzgve.fsf@suse.de/>.
>>
>> To be honest, I think relatively basic standard C aspects should be known
>> to people, and "rejects a declaration right after a case label" is enough
>> of a pointer imo.
>
> I don't expect everyone to track or remember all the individual features
> as they are added to standard revisions or compiler implementations, and
> details provided by Andreas are useful in determining the prerequsites.
>
> Indeed the breakage caused in the first place serves as a proof: you'd
> imply the code writer as well the approver should have known this feature
> isn't there in every compiler version supported for building binutils, and
> yet they missed it.
>
> Why do you think it would be preferable to require everyone to chase
> every time something that has been already determined?
Something needs saying, sure, but what you left me unclear with is why what
is being said in the description is insufficient.
>>> FWIW it broke the build with GCC 10.2.1 here (production box, system
>>> compiler).
>>
>> At the risk of coming across blunt, but: You complain about too little
>> detail above, and then you report this issue without providing any detail?
>
> Why would I need to repeat the same exact details as with the original
> report? This is just to confirm that the same issue triggers for versions
> other than 8.5.0 (just as Andreas has indicated). I reckon Andrew Burgess
> also reported breakage with his environment.
IOW "it" in your earlier reply referred to the original patch, not this fix?
In that case the fix could go in, but please confirm, as I fear I'm now
sufficiently confused.
Jan
Hi Jan, Maciej,
I redid some testing, and this build error reproduces with both gcc-9.4.0
and gcc-10.5.0; in both cases it is fixed by this patch.
Please feel free to adjust the commit message as needed when committing,
as well as the break/continue placement discussed earlier.
Thanks a lot,
Mark
On Sun, Jun 28, 2026 at 12:32:53AM +0800, Mark Zhuang wrote:
> Hi Jan, Maciej,
>
> I redid some testing, and this build error reproduces with both gcc-9.4.0
> and gcc-10.5.0; in both cases it is fixed by this patch.
>
> Please feel free to adjust the commit message as needed when committing,
> as well as the break/continue placement discussed earlier.
>
> Thanks a lot,
> Mark
Committed, thanks for your patience.
On 6/28/2026 9:41 PM, Alan Modra wrote:
> On Sun, Jun 28, 2026 at 12:32:53AM +0800, Mark Zhuang wrote:
>> Hi Jan, Maciej,
>>
>> I redid some testing, and this build error reproduces with both gcc-9.4.0
>> and gcc-10.5.0; in both cases it is fixed by this patch.
>>
>> Please feel free to adjust the commit message as needed when committing,
>> as well as the break/continue placement discussed earlier.
>>
>> Thanks a lot,
>> Mark
> Committed, thanks for your patience.
TYVM. I meant to get to this over the weekend, but ran out of time.
jeff
@@ -1791,8 +1791,6 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length)
}
break;
case 'p': /* Vendor-specific (SpacemiT) operands. */
- size_t n;
- size_t s;
switch (*++oparg)
{
case 'V':
@@ -1815,13 +1813,15 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length)
break;
case 'u': /* Integer immediate, 'XpuN@S' ...
N-bit unsigned immediate at bit S. */
- n = strtol (oparg + 1, (char **)&oparg, 10);
- if (*oparg != '@')
- goto unknown_validate_operand;
- s = strtol (oparg + 1, (char **)&oparg, 10);
- oparg--;
- USE_IMM (n, s);
- break;
+ {
+ size_t n = strtol (oparg + 1, (char **)&oparg, 10);
+ if (*oparg != '@')
+ goto unknown_validate_operand;
+ size_t s = strtol (oparg + 1, (char **)&oparg, 10);
+ oparg--;
+ USE_IMM (n, s);
+ break;
+ }
case 'n':
case 'b':
used_bits |= ENCODE_SPACEMIT_IME_UIMM2_SP (-1U);
@@ -4309,8 +4309,6 @@ riscv_ip (char *str, struct riscv_cl_insn *ip, expressionS *imm_expr,
break;
case 'p': /* Vendor-specific (SpacemiT) operands. */
- size_t n;
- size_t s;
switch (*++oparg)
{
case 'V':
@@ -4358,20 +4356,22 @@ riscv_ip (char *str, struct riscv_cl_insn *ip, expressionS *imm_expr,
break;
case 'u': /* Integer immediate, 'XpuN@S' ...
N-bit unsigned immediate at bit S. */
- n = strtol (oparg + 1, (char **)&oparg, 10);
- if (*oparg != '@')
- goto unknown_riscv_ip_operand;
- s = strtol (oparg + 1, (char **)&oparg, 10);
- oparg--;
- my_getExpression (imm_expr, asarg, force_reloc);
- check_absolute_expr (ip, imm_expr, false);
- if (!VALIDATE_U_IMM (imm_expr->X_add_number, n))
- as_bad (_("improper immediate value (%"PRIu64")"),
- imm_expr->X_add_number);
- INSERT_IMM (n, s, *ip, imm_expr->X_add_number);
- imm_expr->X_op = O_absent;
- asarg = expr_parse_end;
- continue;
+ {
+ size_t n = strtol (oparg + 1, (char **)&oparg, 10);
+ if (*oparg != '@')
+ goto unknown_riscv_ip_operand;
+ size_t s = strtol (oparg + 1, (char **)&oparg, 10);
+ oparg--;
+ my_getExpression (imm_expr, asarg, force_reloc);
+ check_absolute_expr (ip, imm_expr, false);
+ if (!VALIDATE_U_IMM (imm_expr->X_add_number, n))
+ as_bad (_("improper immediate value (%"PRIu64")"),
+ imm_expr->X_add_number);
+ INSERT_IMM (n, s, *ip, imm_expr->X_add_number);
+ imm_expr->X_op = O_absent;
+ asarg = expr_parse_end;
+ continue;
+ }
case 'n': /* Xpn: stride (0-1), paired with Xpx. */
my_getExpression (imm_expr, asarg, force_reloc);
check_absolute_expr (ip, imm_expr, false);
@@ -924,14 +924,18 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info
switch (*++oparg)
{
case 'd':
- unsigned vd = EXTRACT_OPERAND (SPACEMIT_IME_VD, l) * 2;
- print (info->stream, dis_style_register, "%s",
- riscv_vecr_names_numeric[vd]);
+ {
+ unsigned vd = EXTRACT_OPERAND (SPACEMIT_IME_VD, l) * 2;
+ print (info->stream, dis_style_register, "%s",
+ riscv_vecr_names_numeric[vd]);
+ }
break;
case 's':
- unsigned vs = EXTRACT_OPERAND (SPACEMIT_IME_VS1, l) * 2;
- print (info->stream, dis_style_register, "%s",
- riscv_vecr_names_numeric[vs]);
+ {
+ unsigned vs = EXTRACT_OPERAND (SPACEMIT_IME_VS1, l) * 2;
+ print (info->stream, dis_style_register, "%s",
+ riscv_vecr_names_numeric[vs]);
+ }
break;
case 'm':
{