This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
[PATCH] RISC-V: Accept version and more than one NSE for -march.
- From: Kito Cheng <kito dot cheng at gmail dot com>
- To: "patches at groups dot riscv dot org" <patches at groups dot riscv dot org>
- Cc: "binutils at sourceware dot org" <binutils at sourceware dot org>, Palmer Dabbelt <palmer at sifive dot com>
- Date: Fri, 23 Nov 2018 22:43:27 +0700
- Subject: [PATCH] RISC-V: Accept version and more than one NSE for -march.
- References: <CA+yXCZAd=7z-CL2LueaNPEYER25S7uEVq3KXoiFuEMWHyLzqxQ@mail.gmail.com> <CAFyWVabRJn3dv5av63w_TVpNay1PYWj-DP2wB-TOu9Bi0_1e6w@mail.gmail.com>
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 :)