This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][MC] Adjust conditions to emit R_RISCV_ADD*/R_RISCV_SUB* pairs

Authored by MaskRay on Mar 6 2023, 11:10 PM.



D132262 tried to simplify IsMetadataOrEHFrameSection originally introduced in
D127549 but caused a regression as .quad directives in

.section .note,"a",@note; note:
.quad extern-note    # extern is undefined

.section .rodata,"a",@progbits; rodata:
.quad extern-rodata  # extern is undefined

.section .nonalloc,"",@progbits; nw:
.quad extern-nw

are incorrectly rejected: these differences may be link-time constants and
are allowed in GNU assembler and LLVM MC's non-RISC-V ports.

Relax the conditions to allow these cases. For A-B, A may be defined later, but
this requiresFixups call has to eagerly make a decision. For now, emit ADD/SUB
unless A is .L*. This euristic handles many temporary label differences for
.debug_* and .apple_types sections. Ideally we should delay the decision of
PC-relative vs ADD/SUB until A is defined.

Diff Detail

Event Timeline

MaskRay created this revision.Mar 6 2023, 11:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2023, 11:10 PM
MaskRay requested review of this revision.Mar 6 2023, 11:10 PM
compnerd added inline comments.Mar 7 2023, 7:29 AM

This really seems like the original cases were correct but need to be augmented further to handle data sections. If there is a cross-section textual reference, that should generate the relocation pair. The data section is interesting because the .eh_frame, .apple_names, .apple_types, and .debug_* do not want the pairwise relocation. Adjusting this to handle it that way would not only fix this, but also likely fix the cases of fission that we currently fail on.

MaskRay added inline comments.Mar 7 2023, 12:13 PM

I think we want to avoid reliance on isText() (Kind as set by MCContext::getELFSection). What are the changes you envision?

I think it's right to hard code .eh_frame, .apple_names, and .apple_types for now, and don't try using a generic rule to handle them. .eh_frame may possibly be removed if we can fix MCDwarf.cpp (not sure how much effort it requires).

compnerd added inline comments.Mar 8 2023, 7:53 AM

I think that we should prefer isText over the section name checks. RISCV seems to want to prefer the pair wise relocation in all "text" cases and in very specific data cases (as .debug_*, .eh_frame, .apple_names, .apple_types etc are all data sections where we don't want the pair wise relocation).

MaskRay added inline comments.Mar 8 2023, 10:05 AM

I prefer avoiding isText check. It relies on .Default(SectionKind::getText()); in MCContext::getELFSection which I am still unsure.
In GNU assembler, .rodata and .note sections use R_*_{ADD,SUB}* as well.

The code as-is appears to capture the GNU assembler behavior best, though perhaps there may be some cases that should be rejected while we allow. Hence I say "I suspect we may allow some cases that should be rejected, but this is better than incorrect rejection." in the last sentence of the summary.

MaskRay updated this revision to Diff 504334.Mar 10 2023, 9:15 PM
MaskRay edited the summary of this revision. (Show Details)

Fix -gdwarf-aranges

MaskRay added inline comments.Mar 10 2023, 9:24 PM

OK, I agree that we should use isText here, though I think we should revisit the .Default(SectionKind::getText()) decision in
MCContext::getELFSection for your D133456.

For .long w1-w in the test, gas doesn't emit a pair of ADD/SUB relocations. We currently do and the patch retains the behavior. To properly fix it and match gas, we should delay the relocation decision, which is difficult to implement and definitely out of the scope of this patch.

compnerd accepted this revision.Mar 14 2023, 8:43 AM

Yeah, I think that we will have to slowly erode the differences, the model for the pair-wise relocations here is a challenge.

This revision is now accepted and ready to land.Mar 14 2023, 8:43 AM
This revision was landed with ongoing or failed builds.Mar 14 2023, 3:17 PM
This revision was automatically updated to reflect the committed changes.