[PATCH] D45118: Initial propototype for mergin (custom) debug sections with DWARF format.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

I don't think I understand what you are trying to achieve with this patch in the first place, as you didn't explain it anywhere. Can you write a patch description properly so that we can understand the intention of the change and what the change is?



================
Comment at: wasm/InputChunks.h:186
 
+  const WasmSection &getSection() { return Section; }
+  size_t getPayloadSize() const { return Payload.size(); }
----------------
This function looks a bit odd. This is "InputSection", so an instance of this class is a section. And does a section has "getSection" method? It doesn't sound quite right.


================
Comment at: wasm/InputChunks.h:187
+  const WasmSection &getSection() { return Section; }
+  size_t getPayloadSize() const { return Payload.size(); }
+
----------------
We don't use the term "Payload" in lld. We simply call it Data when we have to name it.

Looks like this function is unused? Can you remove this?


================
Comment at: wasm/InputFiles.cpp:46
 
+static size_t getFunctionSizeLength(const ArrayRef<uint8_t> FunctionBody) {
+  unsigned Count;
----------------
getSizeLength sounds a odd name. Aren't size and length synonyms?


================
Comment at: wasm/InputFiles.cpp:47
+static size_t getFunctionSizeLength(const ArrayRef<uint8_t> FunctionBody) {
+  unsigned Count;
+  llvm::decodeULEB128(FunctionBody.data(), &Count);
----------------
Let's be consistent. Since this function returns size_t, it should be size_t.

But I think it is better to use uint64_t when something can be larger than uint32_t.


================
Comment at: wasm/InputFiles.cpp:134
+    if (auto *Sym = dyn_cast<DefinedFunction>(getFunctionSymbol(Reloc.Index))) {
+      const uint8_t FunctionSizeLength =
+          getFunctionSizeLength(Sym->Function->getFunctionBody());
----------------
Are you sure that uint8_t is enough?

We don't normally add `const` when it is obvious.


================
Comment at: wasm/InputFiles.cpp:141
+    if (auto *Sym = dyn_cast<DefinedSection>(getSectionSymbol(Reloc.Index))) {
+      const auto &SyntheticSection =
+          Symtab->SyntheticSections[Sym->SectionIndex];
----------------
We don't use `auto` unless it's type is obvious.


================
Comment at: wasm/InputFiles.cpp:187
     const WasmSection &Section = WasmObj->getWasmSection(Sec);
     if (Section.Type == WASM_SEC_CODE)
       CodeSection = &Section;
----------------
When you use {}, add braces to all `if` and `else if`s.

I.e.

  if (...) {
  } else if (...) {
  } else {
  }

or

  if (...) ...;
  else if (...) ...;
  else ...;

are OK, but 

  if (...)
  else if (...)
  else if (...) {
  } else ...

isn't.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D45118