[PATCH] D61767: [llvm-elfabi] Emit ELF header and string table section

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich marked an inline comment as done.
jakehehrlich added inline comments.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:32
+
+using namespace llvm::elfabi;
 
----------------
echristo wrote:
> What's with the change?
There was a whole bunch of stuff that was marked as 'static' here but the types and methods should all be "static" as well so I put them in an anonymous namespace and moved the important bits into the same nested namespace below.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:58
+// This type is not currently threadsafe.
+template <class T> class Lazy {
+private:
----------------
echristo wrote:
> The name is somewhat inscrutable, perhaps something else?
Will do, how does "BuildNode" sound? Or "DelayedStep"? Any recommendations?

For those familiar with lazy evaluation this is an easy to understand name but "Thunk" would also be acceptable for that crowd.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:194
   if (Dyn.SONameOffset.hasValue() && *Dyn.SONameOffset >= Dyn.StrSize) {
-    return createStringError(
-        object_error::parse_failed,
-        "DT_SONAME string offset (0x%016" PRIx64
-        ") outside of dynamic string table",
-        *Dyn.SONameOffset);
+    return createStringError(object_error::parse_failed,
+                             "DT_SONAME string offset (0x%016" PRIx64
----------------
echristo wrote:
> The reformatting looks like it could be done separately?
Yeah I just applied it to the whole file and it had some unintended consequences. I'll fix it.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:436
+template <class ELFT>
+void initELFHeader(typename ELFT::Ehdr &ElfHeader, uint16_t Machine) {
+  using Elf_Ehdr = typename ELFT::Ehdr;
----------------
echristo wrote:
> There's already a lot of code in ELFWriter to do this, perhaps you could use it? Or at least support::endian::Writer? In that I see where you're going with this, but perhaps it could be done in a different way rather than redoing existing work (there are only so many times we need to have a new way of writing an elf header :)
Can you be more specific about what you have in mind and where it would live? I'd prefer not to depend on MC if possible. I can think of like 4 places where this happens in llvm and all of it looks pretty similar but they all have subtle differences. Like here I hard code a lot of stuff. In llvm-objcopy we mostly copy that all over. In MC some things are generated and others are hard coded.

Only e_ident really contains anything that is universally hardcoded anywhere and even on aspect of that isn't hard coded. Everything else would need to be specified on a per use base if we wanted to abstract it away. But then what the user has to specify for the remainder is equivalent to what they're already doing. Do you perhaps just want an e_ident helper in the libObject ELF code?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61767