[PATCH] D81775: [COFF] Add cg_profile directive and .llvm.call-graph-profile section

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/MC/MCParser/COFFAsmParser.cpp:336
+
+  // Only store non-temporary symbols.
+  if (!FromSym->isTemporary() && !ToSym->isTemporary())
----------------
zequanwu wrote:
> MaskRay wrote:
> > zequanwu wrote:
> > > MaskRay wrote:
> > > > zequanwu wrote:
> > > > > Only store non-temporary symbols pairs.
> > > > > 
> > > > > In ELF, temporary symbols are also stored. I don't get the point of doing that. As I know, temporary symbols will not show up in object file. So, there is no reason to store temporary symbols.
> > > > Temporary symbols are a subcategory of local symbols. I think in ELF, it does not really matter whether a profile edge references a local symbol or not, a temporary symbol or not. A temporary symbol can also encode a profile edge.
> > > > 
> > > > Is there any particular reason you want to exclude temporary symbols from COFF?
> > > From my understanding, since temporary symbols doesn't show up in final binary and the purpose of `.llvm.call-graph-profile`  is to be used to optimize section layouts of final binary, storing temporary symbols doesn't help. 
> > Please drop isTemporary.
> > 
> > For temporary symbols (`.L` prefixed), the streamer can use a symbol representing the defined section. In ELF, this can cause creation of STT_SECTION. See D51047 for a COFF example.
> Okay. I want to do so, but there is a problem. 
> 
> https://reviews.llvm.org/D44965#1115454 says, temporary symbols should get null symbol index. And it is doing so by setting temporary symbol to `Begin` symbol of its section, https://github.com/llvm/llvm-project/blob/master/llvm/lib/MC/MCELFStreamer.cpp#L473, which will later update to null symbol index in `computeSymbolTable` (e.g. 4 in the test case). So, the `.L.local` temporary symbol will not show up when reading with `llvm-readobj --cg-profile`, https://github.com/llvm/llvm-project/blob/master/llvm/test/MC/ELF/cgprofile.s#L96.
> 
> In order to do the same, we need temporary symbols get null symbol indexes (or similar thing) in COFF. Is there a way to do that in COFF? 
I don't know enough about COFF to suggest a solution, but I believe you can leave out temporary for now

* don't exclude isTemporary() here
* then you can move the implemention to the common AsmParser
* ignore isTemporary() symbols in `MCWinCOFFStreamer::emitCGProfileEntry`



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81775