Skip to content

Fix issues associated with incorrect application of R_RISCV_RELAX#1289

Open
Alexey Karyakin (quic-akaryaki) wants to merge 4 commits into
mainfrom
tlsdesc-bug
Open

Fix issues associated with incorrect application of R_RISCV_RELAX#1289
Alexey Karyakin (quic-akaryaki) wants to merge 4 commits into
mainfrom
tlsdesc-bug

Conversation

@quic-akaryaki

@quic-akaryaki Alexey Karyakin (quic-akaryaki) commented Jun 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Two related changes:

Fixes #1276 and #1278.

@lenary

Sam Elliott (lenary) commented Jun 17, 2026

Copy link
Copy Markdown
Member

This is one approach but there might be a simpler one. How about:

Before we call relaxDeleteBytes, we can go through and change the R_RISCV_RELAX that caused us to perform the relaxation (i.e. at the current offset) into an R_RISCV_NONE. This then ensures that when we delete bytes and move the following relocations a little earlier, that we don't have a leftover R_RISCV_RELAX on the "wrong" offset.

This might be simpler than per-target relocation flags? Alternatively we could have an internal relocation that meant "there was a R_RISCV_RELAX here, but it relates to something deleted", which might make reconstructing reasonable output for emit-relocs easier.

@quic-akaryaki

Alexey Karyakin (quic-akaryaki) commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

This is one approach but there might be a simpler one. How about:

Before we call relaxDeleteBytes, we can go through and change the R_RISCV_RELAX that caused us to perform the relaxation (i.e. at the current offset) into an R_RISCV_NONE. This then ensures that when we delete bytes and move the following relocations a little earlier, that we don't have a leftover R_RISCV_RELAX on the "wrong" offset.

This might be simpler than per-target relocation flags? Alternatively we could have an internal relocation that meant "there was a R_RISCV_RELAX here, but it relates to something deleted", which might make reconstructing reasonable output for emit-relocs easier.

There are two approaches, not one? The first one is simple, but it will remove R_RISCV_RELAX from -emit-relocs output. It is debatable whether they should be there, but correctly they are, and lld keeps them too.

The second one (internal temporary relocation) does not seem simper than a flag. It is really easier to check a bit than "what is the next relocation?" every time so the code that deals with R_RISCV_RELAX can be in one location. Adding internal relocations is also not simple.

However, I think a longer-term direction would be to not load R_RISC_RELAX internally at all to reduce memory footprint, and later reconstruct them in -emit-relocs if needed. I looked into this, but we would need a target-specific hook in emitRelocation(). I didn't feel like I want to go that far.

lenary
Sam Elliott (lenary) reviewed Jun 17, 2026
View reviewed changes
Comment thread lib/Target/RISCV/RISCVLDBackend.h Outdated
lenary
Sam Elliott (lenary) reviewed Jun 17, 2026
View reviewed changes
Comment thread lib/Target/RISCV/RISCVLDBackend.cpp Outdated
quic-seaswara
Shankar Easwaran (quic-seaswara) reviewed Jun 17, 2026
View reviewed changes

@quic-seaswara Shankar Easwaran (quic-seaswara) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we need to deal with creating a unordered map of Relocation * to bool.

Comment thread include/eld/Readers/Relocation.h Outdated
Comment thread include/eld/Readers/Relocation.h Outdated
@quic-akaryaki Alexey Karyakin (quic-akaryaki) marked this pull request as draft June 22, 2026 19:49
@quic-akaryaki Alexey Karyakin (quic-akaryaki) force-pushed the tlsdesc-bug branch 3 times, most recently from 1bad100 to af339ee Compare June 22, 2026 22:04
@quic-akaryaki Alexey Karyakin (quic-akaryaki) changed the title Add backend flags in Relocation and fix incorrect TLSDESC relaxation Fix issues associated with incorrect application of R_RISCV_RELAX Jun 22, 2026
@quic-akaryaki Alexey Karyakin (quic-akaryaki) marked this pull request as ready for review June 22, 2026 22:53
@quic-akaryaki Alexey Karyakin (quic-akaryaki) marked this pull request as draft June 23, 2026 14:49
quic-akaryaki
Alexey Karyakin (quic-akaryaki) commented Jun 23, 2026
View reviewed changes
Comment thread lib/Target/RISCV/RISCVLDBackend.h
/// A map to keep track of the relocation that defines the base address for
/// relative relocations. This is a concept in RISC-V and applies to
/// relocations consisting of a HI20 and LO12 pairs.
llvm::DenseMap<const Relocation *, const Relocation *> m_BaseRelocs;

@quic-akaryaki Alexey Karyakin (quic-akaryaki) Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to make the pointed relocation pointer non-const because R_RISCV_QC_E_32 relaxation updates the access relocation and m_BaseRelocs are used to store pointers to access relocation for it.

@quic-akaryaki

Alexey Karyakin (quic-akaryaki) commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

I the latest push, I reverted the behavior change to apply R_RISCV_RELAX to all relocations at one offset. Now and as before, R_RISCV_RELAX is only applied to the previous relocation.

@quic-akaryaki Alexey Karyakin (quic-akaryaki) marked this pull request as ready for review June 23, 2026 15:57
quic-seaswara
Shankar Easwaran (quic-seaswara) reviewed Jun 23, 2026
View reviewed changes

@quic-seaswara Shankar Easwaran (quic-seaswara) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some initial comments, will review in detail shortly.

Comment thread lib/Target/RISCV/RISCVLDBackend.h Outdated
Comment thread lib/Target/RISCV/RISCVLDBackend.h
llvm::DenseMap<const Relocation *, Relocation *> m_BaseRelocs;

/// Identify relocations that have an associated R_RISCV_RELAX.
llvm::DenseSet<const Relocation *> m_RelocsWithRelax;

@quic-seaswara Shankar Easwaran (quic-seaswara) Jun 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be llvm:DenseSet<Relocation *> ?

@quic-akaryaki Alexey Karyakin (quic-akaryaki) Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like keep the const because there is no need to update it through this set.

Comment thread lib/Target/RISCV/RISCVLDBackend.cpp
Comment thread lib/Target/RISCV/RISCVLDBackend.cpp
Comment thread lib/Target/RISCV/RISCVLDBackend.cpp Outdated
@quic-akaryaki

Alexey Karyakin (quic-akaryaki) commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

Sam Elliott (@lenary) ping pls.

@quic-akaryaki
[RISC-V][NFC] Refactor relaxation loop
13816d1 
The relaxation loop is rewritten to test the "phase" in the outer
layer and relocation type in the inner layer. This way the flow is
clearer and it is easier to make modifications based on the type
of relaxation. It may be also faster as we do not do multiple-choice
tests when they are inapplicable. Moreover, the whole loop can now be
rewritten as an explicit sequence of phases, if we want to.

Signed-off-by: Alexey Karyakin <akaryaki@qti.qualcomm.com>
@quic-akaryaki
[RISC-V] Correctly associate relocations with R_RISC_RELAX
8e36cfe 
Previously, finding out if a relocation has an associated R_RISCV_RELAX
was done by searching the list of relocations by offset. This led to
picking a wrong R_RISCV_RELAX during relaxation because relocation
offset can change during relaxation.

The bug affects TLSDESC relocations and R_RISCV_QC_E_32/R_RISCV_QC_ACCESS_32/16
pairs (which were added just now) because their relaxation options
depend on R_RISCV_RELAX attached to a different relocation.

Create a side set to mark relocations that have R_RISCV_RELAX associated
with them. The association has to be established after relocations are
sorted by offset but before relaxation.

It would be more efficient to add a flag in the Relocation object but
that would increase it size. One option would be to shrink the type
field, but unfortunately in ELF64 it has to be 32-bit.

Fixes #1278 and #1276.

Signed-off-by: Alexey Karyakin <akaryaki@qti.qualcomm.com>
@quic-akaryaki
[RISC-V][NFC] Redo handling of "inverted" relocation pairs
77982e4 
Associate pcrel_lo/tlsdesc_lo relocations to the corresponding pcrel_hi
tlsdesc_hi relocations after all relocations are sorted by offset. This
allows us to remove the "pending" relocation list and create all
relocations in the original order.

This fixes the quadratic run time when every search for the pcrel_hi
relocation required a full scan of the relocation list (three times!).
the findRelocation() function is removed from ELFSection because it was
only used for one purpose in the RISC-V backend only.

Finding the vendor relocation still requires a linear search because
handleVendorRelocation() can override creating the relocation by
returning true, so this can't be moved after sorting.

Signed-off-by: Alexey Karyakin <akaryaki@qti.qualcomm.com>
@quic-akaryaki
[NFC] Remove LastVisit argument and default addend handleRelocation
7f487df 
LastVisit was only used in the RISC-V backed to handle "pending"
relocations. The addend is important and should not be omitted,
and it never is.

Signed-off-by: Alexey Karyakin <akaryaki@qti.qualcomm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lenary Sam Elliott (lenary) lenary left review comments
@quic-seaswara Shankar Easwaran (quic-seaswara) quic-seaswara left review comments
@jonathonpenix Jonathon Penix (jonathonpenix) Awaiting requested review from jonathonpenix
@parth-07 Parth (parth-07) Awaiting requested review from parth-07 parth-07 is a code owner
@quic-areg quic-areg Awaiting requested review from quic-areg quic-areg is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

eld reorders PCREL_LO12_*/RELAX relocs when the LO is a forward reference

3 participants

@quic-akaryaki @lenary @quic-seaswara