Wasm: section kind can be code

[ncw]

Summary

Currently, when creating a named "section", the Wasm frontend forces it to use SectionKind::Data, whereas in fact C++ does generate code sections.

I can see how this got here - there's a default implementation in TargetLoweringObjectFile::getKindForGlobal which returns all sorts of different section kinds to save duplicating that for each target. Most of those section types are not appropriate for Wasm.

We do however need to be able to create code sections for C++.

[sbc100]

lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1283 ↗

(On Diff #125737)

Hmm.. makes sense. Do you know why this wasn't been triggered for normal C global functions?

Is a separate function needed here? could be simple ternary operator?

[ncw]

lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1283 ↗

(On Diff #125737)

I think it's because normal C functions don't get put in an explicit section (hence don't go through getExplicitSectionForGlobal). But "special" functions like C++ inline ctors etc have a section specified.

I guess the separate function isn't needed - I just wanted to match the GNU code, and have a comment on each line explaining why it's different to the GNU target. The comments don't add much, but they'd help an LLVM newbie like me at least.

[sbc100]

I wonder if we can add test for this? I guess it doesn't effect the -elf target only the -wasm one?

[ncw]

Yeah, I tried to add a test... but couldn't find where! The problem problem only occurs when the problematic code generated by the frontend goes through the MC layer.

I tried putting a test in tests/CodeGen/WebAssembly, but emitting assembly from llc doesn't reproduce the problem. And, feeding pre-generated input to the MC layer doesn't really exercise the problem either, so I also couldn't reproduce it with tests in test/MC. The test would basically have to be testing both at once, and I'm an LLVM newbie and can't work out where the test should go, since it's really a target-specific LLVM bug (not a problem in LLC or Clang) but testing the parts in isolation doesn't appear to repro the problem.

If it's any consolation, the LLD tests I've added to the .init_array review require this fix to be applied to llc before they pass, so it is tested there.

[sunfish]

lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1276 ↗

(On Diff #125737)

To help me understand how this all fits together, can you describe the circumstances in which this implementation returns a different value than just { return K; } would?

[ncw]

lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1276 ↗

(On Diff #125737)

Some default heuristics (shared between all targets) determine the value of K here. Those come from TargetLoweringObjectFile::getKindForGlobal, which returns all sorts of standard and exotic kinds, like rodata, BSS, etc.

In WebAssembly, we don't have rodata, everything just goes in the linear memory as a "data" chunk, so all data-like section types should be flattened to data. Hence, the original author was nearly-right to throw away the target-independent default section kind, and just go with "data".

Maybe one day WebAssembly will add support for marking segments of the linear memory as read-only (for security/bug-catching), in which case I guess we'd have to feed through rodata SectionKinds to the Wasm streamer.

[sbc100]

I'm gonna see if I can come up with precise test for this, and then will land it.

FYI, for WebAssembly changes we've been using the convention of starting the change title with "[WebAssembly]"

[ncw]

Thanks for looking into the test!

Here's an example of the failing code (won't compile with clang --target=<> -o /dev/null -c test.c):

extern void externalFn(void); // Avoid optimising out ctor calls
struct StaticCtor {
  StaticCtor();
};
// Not inline to avoid a COMDAT:
StaticCtor::StaticCtor() { externalFn(); }

static StaticCtor staticCtor;

llvm/trunk/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

if (!C)
     return;
 
   // TODO(sbc): At some point we may need COMDAT support but currently
   // they are not supported.
   report_fatal_error("WebAssembly doesn't support COMDATs, '" + C->getName() +
                      "' cannot be lowered.");
 }
 
+static SectionKind getWasmKindForNamedSection(StringRef Name, SectionKind K) {
+  // If we're told we have function data, then use that.
+  if (K.isText())
+    return SectionKind::getText();
+
+  // Otherwise, ignore whatever section type the generic impl detected and use
+  // a plain data section.
+  return SectionKind::getData();
+}
+
 MCSection *TargetLoweringObjectFileWasm::getExplicitSectionGlobal(
     const GlobalObject *GO, SectionKind Kind, const TargetMachine &TM) const {
   StringRef Name = GO->getSection();
   checkWasmComdat(GO);
-  return getContext().getWasmSection(Name, SectionKind::getData());
+  Kind = getWasmKindForNamedSection(Name, Kind);
+  return getContext().getWasmSection(Name, Kind);
 }
 
 static MCSectionWasm *selectWasmSectionForGlobal(
     MCContext &Ctx, const GlobalObject *GO, SectionKind Kind, Mangler &Mang,
     const TargetMachine &TM, bool EmitUniqueSection, unsigned *NextUniqueID) {
   StringRef Group = "";
   checkWasmComdat(GO);
 

llvm/trunk/test/MC/WebAssembly/custom-code-section.ll

+; RUN: llc -mtriple wasm32-unknown-unknown-wasm -O2 -filetype=obj %s -o %t.o
+
+; Wasm silently ignores custom sections for code.
+; We had a bug where this cause a crash
+
+define hidden void @call_indirect() section "some_section_name" {
+entry:
+  ret void
+}