In D100835#2707537, @MaskRay wrote:

In D100835#2701062, @jrtc27 wrote:

Why write your own deleteRanges? I took great care in writing mine to ensure there were no hidden quadratic complexities, but your simpler version has introduced them. Just take mine, unless there is a problem with it nobody's mentioned. Same goes for mutableData being a less efficient version of mine.

I also don't like making this overly target-specific. We should take the time to get the interface right, not just punt on it and shove it all into target-specific code.

+1 for a low time complexity implementation. The quadratic time complexity of the binutils approach may run into scalability problems.

I haven't closely looked into this yet, but it seems that R_RISCV_GPREL_* support can be contributed separately.

This addresses some of the comments by @jrtc27:

In D68065#1683706, @MaskRay wrote:

The shrinking section nature may be similar to R_RISCV_RELAX as used by RISC-V. The RISC-V port of binutils ld.bfd has a similar routine to change section sizes and update symbol values. @PkmX and I haven't implemented the relaxation in the lld RISC-V work. This patch inspires me to explore more on the possibility to implement section shrinking and make it general. I don't like the binutils implementation because it is quadratic about several variables.

This should be fine since lld doesn't support linker relaxation, so we just need to fabricate a dummy value for __global_pointer$ that allows it to link.

• Use ET_NONE as e_type in tests.
• Drop the unnecessary dyn suffix in test name.

• -R now only checks for the presence of SHT_DYNAMIC section.
• Reduce dynamic test case: remove unnecessary fields and use R_X86_64_RELATIVE so we don't even need a symbol table.
• Clarify the test for no SHT_DYNAMIC section case.
• Use llvm::any_of as suggested by @grimar.

In D60250#1461308, @MaskRay wrote:

I think the error message error: not a dynamic object is a bit inappropriate. GNU objdump reports the error if it cannot find the dynamic section and the BFD flag DYNAMIC (ET_DYN) is not set. This rules looks weird to me as it can just say it cannot find the .dynamic section. What if the file is an ET_DYN but its .dynamic is missing? It may swallow the error message.

The test case was reduced from a working executable with unnecessary sections/symbols removed, hence the odd addresses and fields. I will specifically craft a minimal yaml for the dynamic test.

Addressed the review comments:

• Rebased onto master
• Moved GotRel into if (Config->Is64)
• Use a literal constant for TLS_DTP_OFFSET.
• Rewrite all tests in MC. However this depends on D55667 to assemble la.tls.ie and la.tls.gd, which is not accepted yet.

Rebased onto master and:

GNU as for RISC-V does actually produce out-of-order relocations so lld cannot assume they are always sorted by offset. This was previously discussed at:

The new patch handles non-dynamic objects logic within llvm-objdump and no longer modifies dynamic_relocation_sections other than making it match on sh_addr. I also added tests for all three cases: ET_DYN (for PIE/shared libraries since there doesn't seem to be an existing test), dynamically linked ET_EXEC and statically-linked ET_EXEC (which should error out).

Modify dynamic_relocation_sections to return Expected and errors out if input is not ET_EXEC/ET_DYN or when no DYNAMIC section is found, the previous revision would just silently do nothing if given a static executable.

• %pcrel_lo that doesn't point to a valid auipc will always produce an error, even if relaxation is enabled. The test pcrel-lo12-invalid.s is modified accordingly.
• getPCRelHiExpr is renamed to getPCRelHiFixup and returns a MCFixup instead, to allow expansion for non %pcrel_hi modifiers in later patches.

@jrtc27 Actually I agree it should produce an error regardless of whether relaxation is enabled, since it is most likely an user error to write a %pcrel_lo that doesn't point to an auipc, and the earlier we can catch it the better.

Yes, getPCRelHiExpr from D54029 needs to modified to also look for fixup_riscv_got_hi20, and if it does find one it should just force a relocation.

Specify that pcrel-lo12-invalid.s will only produce an error with relaxation disabled, otherwise the assembler still emits a relocation which will fail at link time.

Rebased onto trunk.

Rebase onto trunk as the declaration of RISCVAsmBackend is split into its own header file.

Rebased onto trunk.

Rebased onto trunk.

I have a patch available that handles PCREL_LO12_S and is rebased on trunk, if it is okay I will create a separate patch for reviewing.

In D39323#1280971, @ruiu wrote:

Ditto -- if you can write tests in assembly, please convert them from YAML to assembly.

In D39324#1280982, @ruiu wrote:

Did you create these tests files from assembly files? If so, I'd add assembly as comments to keep them, so that we can rewrite them to assembly without re-inventing the same assembly files in the future.

LGTM. I will update D39324 to reflect this change.

In D39324#1280424, @jrtc27 wrote:

Other than my single minor point (and D39323), is there anything blocking this from being accepted?

Ping? This is the last issue blocking lld's RISC-V tests to be rewritten to use llvm-mc and llvm-objdump right now.

Abandoning as this has been implemented by another commit.

@ruiu I've uploaded a new patch that adds the blank line to RISCVGlobalPointer as requested. As I do not have commit rights to the LLD repository, could you commit the patch if there are no last-minute issues.

In D39322#1190218, @ruiu wrote:

Apologies for my belated response. I think this patch is generally looking good. One thing I'd fix before submitting is to rewrite a test using assembly. We do not usually use YAML file for testing. Is there any reason you needed to write them in YAML?

Rebased. NFC from last revision.

RISCV now writes its own GOT header, with one entry that stores the address of .dynamic section. _GLOBAL_OFFSET_TABLE_ should be set to the head of .got so that _GLOBAL_OFFSET_TABLE_[0] == &_DYNAMIC.

I've rebased the diff to latest trunk again. Some notable differences from last revision:

Rebase onto master.

Rebase onto master.

Rebase onto master.

Rebase onto latest master.

Rebase onto latest master.

Rebase onto latest master.

Implement the -R option as an alias to either --rpath or --just-symbols. It is treated as latter if the argument file exists and is not a directory.

Added a check to only allow ET_EXEC and the corresponding test.

• Copy symbol type, size, binding, etc, like D39348.
• Update test cases.

In D39348#950848, @grimar wrote:

What is your use case to specify only single --just-symbol file ?

If the only input file is specified as the argument of --just-symbol, lld will error out (ld.bfd accepts this):

Oops. It looks like I actually didn't submit some of these draft comments.

Rebase onto latest master

• Rebase onto latest master
• Clean up #includes

• Rebase onto latest master
• Clean up #includes

clang-format-diff RISCV::writePLT.

Make R_RISCV_CALL reuse R_RISCV_PCREL_HI20 and R_RISCV_PCREL_LO12_I.

I believe the best place would be either the issue tracker on the psABI doc, or RISC-V's sw-dev mailing list.

• Mark TLS_DTP_OFFSET as internal linkage.

• Use write32le.
• Since __global_pointer$ is no longer absolute, remove workaround to make it a static link-time constant in PIE.

• Use {read,write}32le to read/write insturctions.
• Make __global_pointer$ non-absolute.

De-template RISCV class.

De-template RISCV class and assume little-endian.

• Renamed uint32_t to RelType.
• Detemplate the RISCV class.

In D39322#908393, @ruiu wrote:

As you said this looks like a pretty straightforward port to RISC-V. Thank you for doing this!

IIRC, last time when we discussed RISC-V support in lld, I had a concern that linker relaxation (moving code inside a section to make it compact) is virtually mandatory to link RISC-V object files. What is the situation of it? If you can link a small static executable with this patch, then it is probably no longer mandatory, but I want to make sure that that's the case.

llvm-readobj would also need to be modified to recognize RISC-V eflags, and I think it is better to keep that in a separate commit.

Added RISC-V tests for ObjectYAML