[PATCH] D74531: [WebAssembly] Emit PCH __clang_ast in custom section

[Yuta Saito via Phabricator via llvm-commits]

kateinoigakukun marked 3 inline comments as done.
kateinoigakukun added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1701
+  if (Name == "__clangast")
+    return SectionKind::getMetadata();
+
----------------
sbc100 wrote:
> sbc100 wrote:
> > 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.
> 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.
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.


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:383
+    uint64_t RoundedUpLength = (MinLength + 3ULL) & ~3ULL;
+    encodeULEB128(Name.size(), W.OS, 5 + (RoundedUpLength - MinLength));
+    W.OS << Name;
----------------
sbc100 wrote:
> 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 .
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?


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:1113
+  if (Sym.getName() == "__clang_ast")
+    return false;
+
----------------
sbc100 wrote:
> Why this part needed ?  What happens if you don't specify this?  I looks like removes section symbol from the symtab?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74531/new/

https://reviews.llvm.org/D74531