[PATCH] D89432: [llvm-elfabi] Emit ELF .dynsym and .dynamic sections

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:150
+    Entries[Index] = Entry;
+  }
+
----------------
haowei wrote:
> grimar wrote:
> > Can't this be the following?
> > 
> > ```
> >   void modifyAddr(size_t Index, uint64_t Addr) {
> >     Entries[Index].d_un.d_ptr = Addr;
> >   }
> > ```
> > 
> > (the same below)
> Fixed in latest diff.
Not fixed. I suggested a one line method.



================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:233
+      uint16_t Shndx = Sym.Undefined ? SHN_UNDEF : 1;
+      DynSym.Content.add(StrTab.Content.getOffset(Sym.Name), Sym.Size, Bind,
+                         (uint8_t)Sym.Type, 0, Shndx);
----------------
haowei wrote:
> grimar wrote:
> > I am not sure what is happening here. Could you explain?
> > Why a symbol is attached to `.text` and why it is assumed that `.text` has index 1?
> > Also, where it is tested?
> This snippet adds symbols from the ELFStub structure into the .dynsym section builder. The "1" here is a placeholder. It does not mean ".text" has to be index 1. The issue here is for non-Undefined symbols, the shndx needs to be a value other than 0. Please correct me if I am wrong, at link time, for dynamic linking, the shndx value does not matter as long as it is not SHN_UNDEF for non-Undefined symbols, that why I put 1 here, which in current state, it points to ".dynsym" itself.
> 
> The TODO message means I would like to add a dummy place holder ".text" section and point symbols to it in the stub SO in case any tool need a valid shndx value. For linking with ld/lld, I don't think that is necessary.
> 
> I added section name in the binary-write-sheaders.test to verify shndx is filled with expected value. I do want to add some integration test to verify this elfabi tool will work with clang and lld, but it looks like I don't have access to the lld in the llvm test. If you have any good suggestion for integration testing, please let me know.
> 
> We did performed end to end testing with elfabi tool in Fuchsia build and the linked binary from stub SO were equivalent to the ones linked from original SO files. If we chose to preserve the order of symbol table, the build results were the same bit by bit.
> Please correct me if I am wrong, at link time, for dynamic linking, the shndx value does not matter as long as it is not SHN_UNDEF for non-Undefined symbols, that why I put 1 here, which in current state, it points to ".dynsym" itself.

I guess you are right. I can't imagine a good reason why `shndx` can be important in run time. Perhaps someone else who are more familar with the logic of existent dynamic loaders can confirm.

> The TODO message means I would like to add a dummy place holder ".text" section and point symbols to it in the stub SO in case any tool need a valid shndx value.

I think you should use "point" word, not "points" then (in the TODO comment). Now the comment sounds like it already points to `.text`.

Also, I think it worth to update this comment to explain that the symbol index is not really important and you assign an arbitrary non-SNH_UNDEF value, which now points to `.dynsym` itself.

I am not sure how much is useful to mention ".text" here (i.e. how much usefull would be to add this section and TODO about that plan).
As far I understand there is no known tool that is known to not like the existent behavior.

> I added section name in the binary-write-sheaders.test to verify shndx is filled with expected value.

I think it is enough. The behavior of LLD is tested separatelly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89432