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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


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



================
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;
----------------
kateinoigakukun wrote:
> 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?
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.


================
Comment at: llvm/test/MC/WebAssembly/clangast.ll:29
+; CHECK:    Type: CUSTOM (0x0)
+; CHECK:    Size: 4
+; CHECK:    Offset: 97
----------------
I'm confused by this size value.  Shouldn't it be at least 27?


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

https://reviews.llvm.org/D74531