[PATCH] D68065: Propeller: LLD Support for Basic Block Sections

[Chih-Mao Chen via Phabricator via llvm-commits]

PkmX added a comment.

In D68065#1683706 <https://reviews.llvm.org/D68065#1683706>, @MaskRay wrote:

> The shrinking section nature may be similar to R_RISCV_RELAX as used by RISC-V. The RISC-V port of binutils ld.bfd has a similar routine to change section sizes and update symbol values. @PkmX and I haven't implemented the relaxation in the lld RISC-V work. This patch inspires me to explore more on the possibility to implement section shrinking and make it general. I don't like the binutils implementation because it is quadratic about several variables.


RISC-V relaxation would require a more complicated framework since relaxation can happen in the middle of sections, but it is still possible to do it in a mostly linear fashion.

----

About the patch itself: There are no tests?



================
Comment at: lld/ELF/SyntheticSections.cpp:2061
   assert(this->type != SHT_DYNSYM || !b->isLocal());
+  assert(this->type != SHT_DYNSYM || !b->isLocal());
+  StringRef SName = b->getName();
----------------
Duplicate `assert`.


================
Comment at: lld/ELF/Target.h:87
 
+  virtual bool deleteFallThruJmpInsn(InputSection &IS, InputFile *File,
+                                     InputSection *NextIS) const {
----------------
Document the meaning of the `bool` returned.


================
Comment at: lld/ELF/Writer.cpp:1647
+static void fixSymbolsAfterShrinking() {
+  for (InputFile *File : objectFiles) {
+    parallelForEach(File->getSymbols(), [&](Symbol *Sym) {
----------------
I don't think you need to loop over all input files, just files with sections that have been shrunk.


================
Comment at: lld/ELF/Writer.cpp:1663
+
+      if (Def->value > NewSize) {
+        LLVM_DEBUG(llvm::dbgs() << "Moving symbol " << Sym->getName() <<
----------------
MaskRay wrote:
> This is a dangerous operation. Are you arbitrarily changing st_value/st_size? In what conditions can this be triggered?
I think having symbols in the part of section that is shrunk should just be an error, or at least it should clamp them to the end of a section.


================
Comment at: lld/ELF/Writer.cpp:1738
+        script->assignAddresses();
+    } while (AnyChanged);
+
----------------
Let the user specify how many rounds to run? I suppose the last few rounds may have marginal benefits.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D68065