[WebAssembly] Emit clangast in custom section aligned by 4 bytes

[kateinoigakukun]

Summary

Emit __clangast in custom section instead of named data segment
to find it while iterating sections.
This could be avoided if all data segements (the wasm sense) were
represented as their own sections (in the llvm sense).
This can be resolved by https://github.com/WebAssembly/tool-conventions/issues/138

And the on-disk hashtable in clangast needs to be aligned by 4 bytes,
so add paddings in name length field in custom section header.

The length of clangast section name can be represented in 1 byte
by leb128, and possible maximum pads are 3 bytes, so the section
name length won't be invalid in theory.

Fixes https://bugs.llvm.org/show_bug.cgi?id=35928

[sbc100]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1701

Hmm.. it looks like this function isn't actually needed: https://reviews.llvm.org/D74565.

Does this change work if you simply return K here? I guess/hope it does.

llvm/lib/MC/WasmObjectWriter.cpp

383

I general this seems like trick that could work. However, I think this value is encoded as an LEB32 which means its max length is technically 5. I don't think a 6 byte LEB is valid .

1113

Why this part needed ? What happens if you don't specify this? I looks like removes section symbol from the symtab?

[sbc100]

Can we get a test to see the effect of this patch?

[sbc100]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1701

Let me know if this change is uneeded with https://reviews.llvm.org/D74565.? I'll hold of landing that until I hear back. Hopefully it will just work and simplify this patch once it lands.

[kateinoigakukun]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1701

Thanks for suggestion. But this special treatment is necessary to put the clangast in custom section.
The section kind K come from TargetLoweringObjectFile::getKindForGlobal which is called from AsmPrinter::EmitGlobalVariable, but the getKindForGlobal infers that __clangast global variable should be put in readonly section.

Then readonly section is put in data segment for WebAssembly. But what we really want in WebAssembly context is to put clangast metadata in custom section.

Should I change getKindForGlobal to return metadata section kind for clangast? But I'm not sure how the change effects to other object formats.

llvm/lib/MC/WasmObjectWriter.cpp

383

Thanks. I see, I confirmed that you are right https://webassembly.github.io/spec/core/binary/values.html#integers

Then how should I pad section start? Should I pad with empty data segment or something else?

1113

This part prevents the clangast section symbol from putting it in symbol table.
This part is necessary to put clangast in custom section because the sym is treated as data type but the actual content is not in data section, so WasmObjectWriter raised abortion with "data symbols must live in a data section: __clang_ast" when it tries to write the sym in symtab.

[kateinoigakukun]

Could you take a look again?

[sbc100]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1701

I see. I'd still rather get rid of this function completely in the long run, so if I land my change first you can just do this check directly in TargetLoweringObjectFileWasm::getExplicitSectionGlobal. Is there some other way we can identify __clang_ast here other than by section name? Is it a special kind of symbol?

In the long run I think this problem will be solved by putting each symbol in its own wasm section but to do that we need to diverge from the spec a little. Would you mind added a TODO here and referencing this bug: https://github.com/WebAssembly/tool-conventions/issues/138.

I think it we fix this bug we won't need this change.

It won't solve the alignment issue though :(

llvm/lib/MC/WasmObjectWriter.cpp

383

I think the problem is that the wasm file itself doesn't have the concept of alignment of sections, since its not designed to memory mapped in that way. Its designed to be parsed. Is the loading code expecting the file offset of the section to be aligned? I'm not sure how we can work around this.

llvm/test/MC/WebAssembly/clangast.ll

29 ↗

(On Diff #245051)

I'm confused by this size value. Shouldn't it be at least 27?

[sbc100]

My colleague pointed out that one way to archive padding in the binary format would be to create a synthetic custom section that precedes that one you want to align. Its kind of horrible, but wasm files were not designed to used memory mapped like this.

[kateinoigakukun]

In D74531#1885606, @sbc100 wrote:

My colleague pointed out that one way to archive padding in the binary format would be to create a synthetic custom section that precedes that one you want to align. Its kind of horrible, but wasm files were not designed to used memory mapped like this.

Thanks! I'll try this way. My only concern is whether the order of custom sections are guaranteed.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1701

OK, I'll update after https://reviews.llvm.org/D74565 merged.

Is there some other way we can identify __clang_ast here other than by section name? Is it a special kind of symbol?

I couldn't find any other way to identify the section. I think it's a really special case to support PCH.

In the long run I think this problem will be solved by putting each symbol in its own wasm section but to do that we need to diverge from the spec a little. Would you mind added a TODO here and referencing this bug: https://github.com/WebAssembly/tool-conventions/issues/138.

I see. I agree with you. I'll add TODO comment here.

[sbc100]

Thanks! I'll try this way. My only concern is whether the order of custom sections are guaranteed.

If you simple create the extra section right there in the WasmObjectFileWriter it will be in the right order. I wasn't recommending creating an extra section at the llvm level.

[sbc100]

I just landed https://reviews.llvm.org/D77115 which does similar thing for the embedded bitcode sections and should simplify this CL a little. Although it doesn't try to do the file offset alignment.

If we can figure out way to do alignment I think that is the only remaining blocker for this.

llvm/lib/MC/WasmObjectWriter.cpp

1113

I think this part might no longer be needed after https://reviews.llvm.org/D77115

[patcheng]

llvm/lib/MC/WasmObjectWriter.cpp

383

BTW, here was an implementation I did way back: https://reviews.llvm.org/D42233

If I remember correct, you could spread the extra padding across multiple integer fields.

[kateinoigakukun]

I'm sorry to keep you waiting. I updated to reduce hacks in backend to emit clangast in custom section and move it to clang.
And emit paddings for alignment to fit into 5 bytes.

[sbc100]

llvm/lib/MC/WasmObjectWriter.cpp

374

I would leave the first sentence alone which that applies to all custom sections. The second part of this sentence (the new part) is maybe not needed since you have a comment already on line 378.

Also, how about keeping the simpler form in the default case, and putting this new code in a new method. e.g.:

if (Name != "__clangast") {
  writeString(Name);
} else {
  writeStringWithAlignment(Name, 4);
}

Then the entire new method can be marked with the TODO to remove it if/when we change the section mapping.

[kateinoigakukun]

Thank you for taking a look again. OK, I extracted writeStringWithAlignment and updated comment lines.

[kateinoigakukun]

Thanks for reviewing! Can you merge this patch? I have no commit access.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

   if (K.isText())
     return SectionKind::getText();
 
+  // Clang precompiled header data isn't needed at runtime; use custom section
+  if (Name == "__clangast")
+    return SectionKind::getMetadata();
+
   // Otherwise, ignore whatever section type the generic impl detected and use
   // a plain data section.
   return SectionKind::getData();

llvm/lib/MC/WasmObjectWriter.cpp

   Section.PayloadOffset = W.OS.tell();
 
   // Custom sections in wasm also have a string identifier.
-  writeString(Name);
+  if (Name != "__clangast") {
+    writeString(Name);
+  } else {
+    // pad section start to nearest 4 bytes for Clang PCH
+    uint64_t MinLength =
+        Section.PayloadOffset + 5ULL /* min ULEB128 length */ + Name.size();
+    uint64_t RoundedUpLength = (MinLength + 3ULL) & ~3ULL;
+    encodeULEB128(Name.size(), W.OS, 5 + (RoundedUpLength - MinLength));
+    W.OS << Name;
+  }
 
   // The position where the custom section starts.
   Section.ContentsOffset = W.OS.tell();

   if (Sym.isSection())
     return false;
 
+  // Clang's precompiled headers are in a separate custom section
+  if (Sym.getName() == "__clang_ast")
+    return false;
+
   return true;
 }