[PATCH] D148836: [X86] Use "l" prefix for data sections under medium/large code model

[James Y Knight via Phabricator via llvm-commits]

jyknight added a comment.

1. Largedata is currently x86-only, and so an alternative would be to move some of this into `llvm/lib/Target/X86/X86TargetObjectFile.cpp`. There is code very much like that in e.g. `llvm/lib/Target/RISCV/RISCVTargetObjectFile.cpp` in order to select the "small data/bss" sections (riscv "small data" is not really the same thing -- it's for placing some data within 12-bit offsets from _global_pointer in the main executable, so that the linker can delete the load of the upper bits by relying on GP already being loaded) . However, I think that large-data sections is a somewhat generic concept which could be used on other architectures in the same way (e.g. it seems like it could be implemented about the same way and be just as useful for aarch64). So probably implementing it generically is OK.

2. `-mcmodel=large` on x86 will need more work for it to be interoperable in the same way that -mcmodel=medium is intended to be. Since it's supposed to allow arbitrarily-large CODE as well, we'd need to also place "large code" into large-code sections, so that you don't break relocations from small-code to small-data (ro or rw), by sticking large-code in between. As such -- until that's going happen, which I think needs a discussion on ABI lists -- I'd like to not change the behavior of -mcmodel=large yet.



================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:631
   if (Kind.isReadOnlyWithRel())
-    return ".data.rel.ro";
+    return IsLarge ? ".ldata.rel.ro" : ".data.rel.ro";
   llvm_unreachable("Unknown section kind");
----------------
This is fine as you have it in the compiler, but I'll just note that we cannot really do RELRO for .ldata.rel.ro sections in the linker, because today's program loaders only permit a single GNU_RELRO segment -- so .ldata.rel.ro has to be treated just like .ldata in the linker -- and thus stay as RW during program execution.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:860
+    if (TM.isLargeData())
+      Flags |= ELF::SHF_X86_64_LARGE;
+  }
----------------
(Assuming we keep all this in the generic code) This should be abstracted so that it can work for !X86 in the future. Or, at least `assert(getTargetTriple().getArch() == Triple::x86_64)` in this block.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148836