Home Original page

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

Kito Cheng kito.cheng@gmail.com
Fri Nov 23 15:43:00 GMT 2018 
Hi Jim:

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

> 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.

> 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 :)
More information about the Binutils mailing list