[lld][WebAssembly] Fix segfault in map file support

[tlively]

Summary

The code previously assumed that getChunk would return a non-null pointer for
every symbol, but in fact it only returns non-null pointers for DefinedFunction
and DefinedData symbols. This patch fixes the segfault by checking whether
getChunk returns a null for each symbol and skipping the mapping output for
any symbols for which it does.

[sbc100]

Can you update the mapfile test to tickle this?

[tlively]

I'm not actually sure how to do that in the .s format, since I found this on a real-world project. Do you know what I should add to the test?

[sbc100]

Do you know that symbol type that causes the crash?

[sbc100]

Maybe a DefinedGlobal would do it? You can define wasm globals in .s files (see lld/test/wasm/globals.s).

[sbc100]

lld/wasm/MapFile.cpp

83

How about if we just have fileOffset default to 0 if chunk is null? I think we still want to include all symbols in the map.

[sbc100]

Same issue? https://bugs.llvm.org/show_bug.cgi?id=48972

[tlively]

Yes, almost certainly. I actually ran into another null pointer dereference and had to add another workaround to my local branch as well. I haven't pursued this patch upstream because I still have not been able to create a test case :(

[sbc100]

Can we get this landed? Did adding a defined global to a .s file work? (I think maybe you can just add a defined global to lld/test/wasm/map-file.s?

[tlively]

Trying again now, but last time I tried that (a while ago) I couldn't reproduce the issue.

[tlively]

• Add test with wasm global, try to print it

[tlively]

Ok, I was able to reproduce one of the issues by using a wasm global this time. However, I'm having trouble figuring out how to fit printing the global to the map file in with the current infrastructure. Comments would be very appreciated.

[sbc100]

Seems reasonable to me. Our map file format is relatively arbitrary AFAICT. Its mostly about conveying the final contents of the linked binary IIUC.

[sbc100]

Is this good to go now?

[tlively]

• Clean up

[tlively]

Yes, I think so. I changed this back to skipping symbols without chunks in the because we don't need to demangle global symbols.

lld/wasm/MapFile.cpp

 // Construct a map from symbols to their stringified representations.
 // Demangling symbols (which is what toString() does) is slow, so
 // we do that in batch using parallel-for.
 static DenseMap<Symbol *, std::string>
 getSymbolStrings(ArrayRef<Symbol *> syms) {
   std::vector<std::string> str(syms.size());
   parallelForEachN(0, syms.size(), [&](size_t i) {
     raw_string_ostream os(str[i]);
-    auto &chunk = *syms[i]->getChunk();
-    uint64_t fileOffset = chunk.outputSec->getOffset() + chunk.outputOffset;
+    auto *chunk = syms[i]->getChunk();
+    // TODO: Handle symbol types besides DefinedFunction and DefinedData
+    if (!chunk)
+      return;
+    uint64_t fileOffset = chunk->outputSec->getOffset() + chunk->outputOffset;
     uint64_t vma = -1;
     uint64_t size = 0;
     if (auto *DD = dyn_cast<DefinedData>(syms[i])) {
       vma = DD->getVirtualAddress();
       size = DD->getSize();
       fileOffset += DD->offset;
     }
     if (auto *DF = dyn_cast<DefinedFunction>(syms[i])) {