[PATCH] D70930: [llvm-objcopy][WebAssembly] Initial support for wasm in llvm-objcopy

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-archive-copy.test:86-87
+      - Index: 1
+        Kind: FUNCTION
+        Name: func2
+        Flags: [  ]
----------------
Minor nit: add an extra space between Kind/Name and their values to match the Index 1 symbol.


================
Comment at: llvm/tools/llvm-objcopy/wasm/Object.h:1
+//===- Object.h -------------------------------------------------*- C++ -*-===//
+//
----------------
dschuff wrote:
> alexshap wrote:
> > I'd prefer to follow the structure which we use for other formats (MachO, COFF, ELF), i.e. put the description of the model into Object.h/Object.cpp (the class Object etc), and put the classes Reader and Writer into separate files, even if they are small at the moment, they might grow in the future, it makes things a little bit easier to read. 
> > 
> I don't really mind doing this, but I will note that ELF also has its Readers and Writers in Object.h
I wouldn't mind somebody changing ELF to split them out too. Having them in the same file always confuses me initially.


================
Comment at: llvm/tools/llvm-objcopy/wasm/Writer.cpp:26-28
+  for (const auto &S : Obj.getSections()) {
+    SectionHeaders.emplace_back();
+    auto &Header = SectionHeaders.back();
----------------
I think we've been trying to get rid of these sort of non-obvious `auto` usages within llvm-objcopy. Whilst I can tell that `S` and `Header` are likely some kind of section and section header respectively, it's not obvious to me what their type is. Please replace them with the concrete type.


================
Comment at: llvm/tools/llvm-objcopy/wasm/Writer.cpp:31
+    encodeULEB128(S.SectionType, OS);
+    bool HasName = S.SectionType == 0;
+    size_t SectionSize = S.getContents().size();
----------------
Does wasm define a constant for this `0` case here?


================
Comment at: llvm/tools/llvm-objcopy/wasm/Writer.cpp:35
+      SectionSize += getULEB128Size(S.Name.size()) + S.Name.size();
+    encodeULEB128(SectionSize, OS, 5);
+    if (HasName) {
----------------
What is  the `5` here? Is there a constant for it somewhere if applicable?


================
Comment at: llvm/tools/llvm-objcopy/wasm/Writer.cpp:40
+    }
+    ObjectSize += SectionSize + 1 + 5;
+  }
----------------
Same sort of comment as above. What are the `1` and `5`?


================
Comment at: llvm/tools/llvm-objcopy/wasm/Writer.cpp:52
+  Ptr = std::copy(Obj.Header.Magic.begin(), Obj.Header.Magic.end(), Ptr);
+  memcpy(Ptr, &Obj.Header.Version, sizeof(Obj.Header.Version));
+  Ptr += sizeof(Obj.Header.Version);
----------------
Does wasm have a concept of endianness that we need to care about here?


================
Comment at: llvm/tools/llvm-objcopy/wasm/Writer.cpp:55
+  // Write each section.
+  for (size_t i = 0; i < SectionHeaders.size(); ++i) {
+    Ptr = std::copy(SectionHeaders[i].begin(), SectionHeaders[i].end(), Ptr);
----------------
`i` -> `I`

Also, I believe the standard LLVM style is:

`for (size_t I = 0, S = SectionHeaders.size(); I < S; ++I)`


================
Comment at: llvm/tools/llvm-objcopy/wasm/Writer.cpp:57
+    Ptr = std::copy(SectionHeaders[i].begin(), SectionHeaders[i].end(), Ptr);
+    const auto &Contents = Obj.getSections()[i].getContents();
+    Ptr = std::copy(Contents.begin(), Contents.end(), Ptr);
----------------
Don't use `auto` here please. It's not clear what the type is again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70930