This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 2/2] RISC-V: Merge ELF attribute for ld


On Fri, Dec 7, 2018 at 4:40 AM Kito Cheng <kito.cheng@gmail.com> wrote:
> This patch implement the RISC-V ELF attribute merge, only implement
> exact arch version match now, we'll implement different merge policy
> later.

riscv_version_mismatch, the function parameters aren't declared in the right
coding style, and likewise for a lot of following functions, parameters should
start on the same line as the open parenthesis, and be indented to match if
carry over to multiple lines

riscv_i_or_e_p uses strcmp, the previous patch uses strcasecmp, if we aren't
canonicalizing names to lowercase somewhere, this should probably use
strcasecmp too

I think a ? in the middle of an error message looks funny, we know that there
is a problem here, we should state the error, not ask a question, I'd change
it to a comma

a complex function like riscv_merge_std_ext should have a comment explaining
what it does, why pin and pout are pointers for instance, it looks like it
parses what it can parse and then stores the unparsed stuff back into pin and
pout for the next function, that should be documented

the first error message has a typo, "tring" -> "string"

should presumably return false after calling _bfd_error_handler

for the base architecture, it is putting IN name, major_version, and
minor_version into merged_subsets, but seems to be ignoring OUT name,
major_version, and minor_version, the code only verifies that the first letter
between IN and OUT are the same, it isn't comparing the version numbers

a few more code style issues, when calling a function, arguments go after the
open parenthesis, not on the next line

in the comnment at the end, "Skip all standard extension." should be
"Skip all standard extensions."

in riscv_merge_arch_attr_info, "is unmatched with" sounds awkward, I'd suggest
"doesn't match"

at the end, why are you passing ARCH_SIZE into riscv_arch_str?  Shouldn't that
be the xlen_in or xlen_out value that we already parsed from the attributes?,
the code doesn't require that xlen_in or xlen_out match ARCH_SIZE, maybe it
should?

riscv_merge_attributes has a comment mentioning IBFD and OBFD, but there is
no OBFD parameter, usually capital letters here indicate a parameter name,
should mention the output bfd in INFO instead, I see that the elf32-arm.c
file has the same bug, maybe a previous version took an OBFD argument and
they forgot to fix the comment, we should get our version right

"that hasn't attribute section" is awkward, I'd suggest "that doesn't have
an attribute section"

there is a comment that refers to "ARM" at the end, suggest RISC-V instead

PT_RISCV_ATTRIBUTES should be in the riscv-elf-psabi-doc pull request

Do we really need PT_RISCV_ATTRIBUTES? The ARM port doesn't have one.  It
doesn't appear that any other target has one either.  This seems unnecessary.

In the bfd ChangeLog entry, I'd suggest "New." instead of "New variable."
because only the first three are variables, the rest are functions.

I get 10 extra ld testsuite failures for riscv64-{elf,linux} targets.  It
looks like the problem is the one I referred to earlier, where ARCH_SIZE is
used instead of the xlen mentioned in the attributes.

A more general question, do we want the attribute support enabled for linux
targets?  It may be more of a distraction than a help there, as we don't
expect that linux systems will support more than one type of processor.  At
the moment the attribute support is enabled for linux targets.

Jim


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]