[PATCH] D40849: CodeGen: support an extension to pass linker options on ELF

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added a comment.

Yes, gABI specifies pointer sized alignment, but that is not what currently occurs.  Everything is 4-byte aligned due to a mis-interpretation of the specification long ago, and now it has fossilized into the reality (yay for compatibility!).

Thinking more about this, while Im still not a fan of the split argument proposal, I think that is something that isn't needed for this part of the implementation.  That will be a separate change to clang which will actually process the pragma and generate the associated metadata.  This implementation should be able to accommodate both.



================
Comment at: include/llvm/BinaryFormat/ELF.h:1384
+  NT_LLVM_LINKER_OPTIONS = 0x2302,
+};
+
----------------
jhenderson wrote:
> This number seems a bit arbitrary. Any particular reason for this value specifically?
No, there is no logical reasoning for this value.  Would another value be preferable?


================
Comment at: test/Feature/elf-linker-options.ll:19
+; CHECK: .byte 0
+; CHECK: .p2align 3
----------------
jhenderson wrote:
> I'm confused. Why is this .p2align 3 and not .p2align 2?
Because I can't copy and paste properly.  This should be `.p2align 2`.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:3464-3469
+    uint32_t ID;
+    const char *Name;
+  } Notes[] = {
+    {ELF::NT_LLVM_LINKER_OPTIONS, "NT_LLVM_LINKER_OPTIONS (linker options)"},
+  };
+
----------------
jhenderson wrote:
> Why not make this a proper map of some kind (e.g. DenseMap), so that you can use a straightforward lookup?
It was the pattern that I had started when I was adding support for other note types in `llvm-readobj`.  This is just following that pattern.  Is a `DenseMap` really better than the linear lookup for a very small number of items (<8)?  If so, I can do that in a subsequent change.


Repository:
  rL LLVM

https://reviews.llvm.org/D40849