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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:232
+  if (B.isInSection())
+    return !IsEHOrAppleSection(B.getSection());
 
----------------
compnerd wrote:
> MaskRay wrote:
> > compnerd wrote:
> > > 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.
> > 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).
> 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).
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145474/new/

https://reviews.llvm.org/D145474