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

[Haowei Wu via Phabricator via llvm-commits]

haowei marked 4 inline comments as done.
haowei added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/ELF.h:60
 
+constexpr uint8_t ELFMAG0 = 0x7f;
+constexpr uint8_t ELFMAG1 = 'E';
----------------
echristo wrote:
> This should at least be near the other magic version parts?
These are removed in latest patch as I don't think they are necessary to be added.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:32
+
+using namespace llvm::elfabi;
 
----------------
jakehehrlich wrote:
> 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.
The namespace change is also removed in latest patch.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:58
+// This type is not currently threadsafe.
+template <class T> class Lazy {
+private:
----------------
jakehehrlich wrote:
> 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.
Lazy class is removed in latest patch to avoid hassles in future maintenance. I think it's a brilliant approach but it quite difficult for someone to figure out the ordering by just reading the code.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:644
+                      ELFTarget OutputFormat) {
+  if (OutputFormat == ELFTarget::ELF32LE) {
+    return writeELFBinaryToFile<ELF32LE>(FilePath, Stub);
----------------
echristo wrote:
> llvm style is no braces for a single line.
Fixed in latest patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61767