[PATCH] RISC-V: Accept version and more than one NSE for -march.

[Kito Cheng]

Hi all:

This patch is first patch of ELF attribute section series, it improve
-march parsing, including support extension with version, more than
one non-standard extension, and survivor extension (s and sx).

And move those parsing logic into bfd, in order to shared between
different components.

ChangeLogs are including in the patch.

[Jim Wilson]

Post by Kito Cheng
This patch is first patch of ELF attribute section series, it improve
-march parsing, including support extension with version, more than
one non-standard extension, and survivor extension (s and sx).
And move those parsing logic into bfd, in order to shared between
different components.

Overall this look OK, but there are a number of minor problems with it
that need to be fixed.

The bfd ChangeLog entry is incomplete, but it just looks like a lot of
missing "New." and "Likewise." entries.

bfd ChangeLog Typo, funciton -> function

In riscv_next_arch_char, np isn't a pointer, so incrementing it in the loop
doesn't make any sense.

In riscv_parsing_subset_version, should mention that it modifies major_version
and minor_version.

"return pointer point to the end of version" is awkward, I'd suggest just
"return pointer to the end of version" or maybe "return value points to the
end of version"

Typo in riscv_supported_std_ext comment, "stadnard extension" ->
"standard extensions".

Typos in riscv_parse_sv_or_non_std_ext comment, "stdandard" -> "standard"
and "Paring" -> "Parsing". Doesn't mention the return value.

Probably all of the function comments should be expanded a bit to explain what
the arguments are for and what the return value is.

In riscv_parse_subset, disallows e and f, but that restriction is being
removed from the v2.2 ISA spec. This is in the existing code so this is OK for
now. Fixing this can be a separate patch, as we probably also need fixes
in other places to make this work, and we probably need an ISA version check
here to still disallow it for v2.1 and earlier.

In gas ChangeLog, doesn't mention new elfxx-riscv.h include. Also doesn't
mention the riscv_after_parse_args changes.

Doesn't handle the new Z extensions, but this could perhaps be a separate
patch, since it isn't in the existing code, and only valid in the v2.2 ISA
spec.

There is a missing function riscv_parsing_ext_version. Looks like this was
renamed to riscv_parsing_subset_version. The patch won't build without this
change.

The type change of xlen_requirement to unsigned causes a build error in
opcodes/riscv-dis.c, which requires changing a local variable xlen to
unsigned. That might depend on the local gcc version, but I needed this
fix on Ubuntu 18.04 with gcc-7.3 to build with the patch applied.

Jim

[Kito Cheng]

Hi Jim:

I am on vocation until Dec, I’ll update the patch soon after back, I am
very appreciate your review :)

Post by Jim Wilson
Overall this look OK, but there are a number of minor problems with it
that need to be fixed.
The bfd ChangeLog entry is incomplete, but it just looks like a lot of
missing "New." and "Likewise." entries.
bfd ChangeLog Typo, funciton -> function
In riscv_next_arch_char, np isn't a pointer, so incrementing it in the loop
doesn't make any sense.
In riscv_parsing_subset_version, should mention that it modifies major_version
and minor_version.
"return pointer point to the end of version" is awkward, I'd suggest just
"return pointer to the end of version" or maybe "return value points to the
end of version"
Typo in riscv_supported_std_ext comment, "stadnard extension" ->
"standard extensions".
Typos in riscv_parse_sv_or_non_std_ext comment, "stdandard" -> "standard"
and "Paring" -> "Parsing". Doesn't mention the return value.
Probably all of the function comments should be expanded a bit to explain what
the arguments are for and what the return value is.
In riscv_parse_subset, disallows e and f, but that restriction is being
removed from the v2.2 ISA spec. This is in the existing code so this is OK for
now. Fixing this can be a separate patch, as we probably also need fixes
in other places to make this work, and we probably need an ISA version check
here to still disallow it for v2.1 and earlier.
In gas ChangeLog, doesn't mention new elfxx-riscv.h include. Also doesn't
mention the riscv_after_parse_args changes.
Doesn't handle the new Z extensions, but this could perhaps be a separate
patch, since it isn't in the existing code, and only valid in the v2.2 ISA
spec.

For Z extension and all other changes in v2.2, I plan to send another patch
for that, and it’ll including new configure option, --with-isa-spec= and
new command line option -misa-spec= to control that.

Post by Jim Wilson
There is a missing function riscv_parsing_ext_version. Looks like this was
renamed to riscv_parsing_subset_version. The patch won't build without this
change.
The type change of xlen_requirement to unsigned causes a build error in
opcodes/riscv-dis.c, which requires changing a local variable xlen to
unsigned. That might depend on the local gcc version, but I needed this
fix on Ubuntu 18.04 with gcc-7.3 to build with the patch applied.

I guess I didn’t found those build error because incremental build, will
becareful next time.

Thansk :)