[PATCH] D45118: Linking debug (DWARF) sections from the WebAssembly object files

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: wasm/InputFiles.cpp:47
+static size_t getFunctionSizeLength(const ArrayRef<uint8_t> FunctionBody) {
+  unsigned Count;
+  llvm::decodeULEB128(FunctionBody.data(), &Count);
----------------
ruiu wrote:
> 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.
I don't think you need "const" with ArrayRef as Refs are immutable already.


================
Comment at: wasm/InputFiles.cpp:83
+    }
+    return 0;
+  default:
----------------
Section symbols are always defined.  So you can just cast here and never return 0.

Also, perhaps getSectionSymbol should just return DefinedSection instead?


================
Comment at: wasm/InputFiles.cpp:341
+  case WASM_SYMBOL_TYPE_SECTION:
+    llvm_unreachable("undefined section symbols must be defined");
   }
----------------
The error message is little confusing.  How about "section symbols cannot be undefined"


================
Comment at: wasm/Symbols.h:199
+  }
+};
+
----------------
sbc100 wrote:
> I don't think we can have undefined section symbols, so maybe we only need one type here?
Hot about this:  We can have single SectionSymbol which we know is always defined.   It can have getOutputSectionIndex() / setOutputSectionIndex() as well as `const InputSection *Section;`.

The Writer could then just pick the first SectionSymbol with a given name to be the in the symbol table?  Then we wouldn't need to create any new symbols and we wouldn't need `CombinedSection` class at all.

Would that work?  I could be missing something.


================
Comment at: wasm/Writer.cpp:408
+             StringRef(OSec->Name).startswith(".debug"))
+      Name = InternedNames.insert("reloc." + OSec->Name).first->getKey();
     else
----------------
Is this just to save the Strings?   If so does `Saver.save()` work?


================
Comment at: wasm/Writer.cpp:497
+      } else
+        assert(false);
     }
----------------
A pattern use elsewhere is to simply to assume you can cast to the ramaining type in the final else.

e.g:

```
} else {
   // At this point we know we have CombinedSection 
   auto *S = cast<CombinedSection>(Sym));
   ...
}
```

That way the cast will fail at runtime if anything goes wrong and you don't need the assert case


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D45118