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

[Yuta Saito via Phabricator via llvm-commits]

kateinoigakukun marked an inline comment as done.
kateinoigakukun added a comment.

In D74531#1885606 <https://reviews.llvm.org/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.



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


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

https://reviews.llvm.org/D74531