[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());
 
----------------
MaskRay wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145474