[PATCH] D45340: [WebAssembly] Add support for user-defined custom sections

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

I think overall it is looking good.



================
Comment at: wasm/InputChunks.h:185
+    unsigned Count;
+    uint64_t NameSize = llvm::decodeULEB128(Section.Content.data(), &Count);
+    PayloadOffset = Count + NameSize;
----------------
If you move this to .cpp, can you remove `#include "LEB128.h"`?

Please add a comment what this code is doing.


================
Comment at: wasm/InputChunks.h:196
+  ArrayRef<uint8_t> data() const override {
+    return Section.Content.slice(PayloadOffset);
+  }
----------------
It may make sense to do this in the ctor and store an ArrayRef instead of an size_t offset as a member.


================
Comment at: wasm/InputChunks.h:198-200
+  uint32_t getInputSectionOffset() const override {
+    return 0; // TODO if we decide to make absolute offset: Section.Offset
+  }
----------------
What is this function? Please add a comment to InputChunk class.


================
Comment at: wasm/OutputSections.h:116
 
+class CustomSection : public OutputSection {
+public:
----------------
Please add a class comment.


================
Comment at: wasm/OutputSections.h:118
+public:
+  explicit CustomSection(std::string Name, size_t Size,
+                         ArrayRef<InputSection *> InputSections);
----------------
I wouldn't add `explicit` because it is hard to accidentally trigger implicit type conversion for this class.


================
Comment at: wasm/Writer.cpp:314
+    // blindly copied
+    if (Name == "linking" || Name == "name" || Name.startswith("reloc."))
+      continue;
----------------
Are you sure that these section names don't start with "."?


================
Comment at: wasm/Writer.cpp:330
+                 << "\n");
+    auto Section = make<CustomSection>(Name, Offset, Sections);
+    OutputSections.push_back(Section);
----------------
You can eliminate this variable.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D45340