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

[zequanwu]

Summary

Add cg_profile directive and .llvm.call-graph-profile section in COFF.
The assembly format and binary are the same as ELF.

[zequanwu]

llvm/lib/MC/MCParser/COFFAsmParser.cpp

336

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.

llvm/tools/llvm-readobj/llvm-readobj.cpp

349–352 ↗

(On Diff #270540)

Probably, these two should be combined into one option --cg-profile.

[MaskRay]

llvm/lib/MC/MCParser/COFFAsmParser.cpp

307

If the code duplicates ELFAsmParser, please move it to AsmParser::

llvm/test/MC/AsmParser/directive_cgprofile.s

2

FileCheck %s (reuse CHECK:)

[jhenderson]

I'd expect a dedicated test for the new llvm-readobj dumping behaviour in "test/tools/llvm-readobj/COFF", probably using yaml as input if the format is supported or the binary content isn't ridiculous. We need to be careful not to create circular testing to ensure that llvm-readobj doesn't share the same bug as MC.

It probably makes sense to split the llvm-readobj dumping changes into a separate patch, that can be done separately, probably in addition to a third patch as mentioned inline for the option.

llvm/tools/llvm-readobj/COFFDumper.cpp

1974 ↗

(On Diff #270540)

I've not looked into whether this is possible, but if it is, it might be nice to share at least some of the COFF and ELF code, since presumably the section format is identical in both cases.

llvm/tools/llvm-readobj/llvm-readobj.cpp

349–352 ↗

(On Diff #270540)

Yeah, rather than adding a coff-specific option, I'd add --cg-profile and make --elf-cg-profile an alias for backwards compatibility (or possibly even delete it, but I'd be somewhat reluctant to do that). You can do that in a separate patch before this one.

Also, the new option, in whatever form it takes, needs adding to the CommandGuide doc for llvm-readobj and llvm-readelf (the latter only for the generic --cg-profile option, and only if applicable).

509 ↗

(On Diff #270540)

By using the new name, you can avoid this code change.

[zequanwu]

• Split llvm-readobj part to another patch.
• Write .llvm-call-graph-profile section data in the same format as ELF.

[zequanwu]

llvm/lib/MC/MCParser/COFFAsmParser.cpp

307

Not exactly the same. See the comment below.

llvm/tools/llvm-readobj/COFFDumper.cpp

1974 ↗

(On Diff #270540)

COFF.h lack some functionalities to do the same parsing as ELF.h. I think this is also why the parsing of Addrsig section in COFF doesn't share any code with ELF, even if the section format is identical in both cases.

[MaskRay]

llvm/lib/MC/MCParser/COFFAsmParser.cpp

336

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?

llvm/lib/MC/MCWinCOFFStreamer.cpp

342

Why setIsWeakExternal?

[zequanwu]

llvm/lib/MC/MCParser/COFFAsmParser.cpp

336

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.

llvm/lib/MC/MCWinCOFFStreamer.cpp

342

Doing the same as finalizeCGProfileEntry in ELF, https://github.com/llvm/llvm-project/blob/master/llvm/lib/MC/MCELFStreamer.cpp#L483.

[MaskRay]

llvm/lib/MC/MCParser/COFFAsmParser.cpp

336

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.

llvm/lib/MC/MCWinCOFFStreamer.cpp

342

OK.

http://llvm.org/docs/Extensions.html#sht-llvm-call-graph-profile-section-call-graph-profile says that the undefined symbol is weak. It does not have to be specified that way...

[zequanwu]

llvm/lib/MC/MCParser/COFFAsmParser.cpp

336

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?

[MaskRay]

llvm/lib/MC/MCParser/COFFAsmParser.cpp

336

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

[zequanwu]

• Move ParseDirectiveCGProfile to MCAsmParserExtension.
• Ignore temporary symbols in MCWinCOFFStreamer::emitCGProfileEntry.

[zequanwu]

Friendly ping.

[MaskRay]

LGTM with one nit.

llvm/test/MC/COFF/cgprofile.s

119

No newline

[zequanwu]

rebase and add newline at end of file.

[zequanwu]

This was landed, but I accidentally wrote wrong diff ID (D83597) in the commit message: https://github.com/llvm/llvm-project/commit/0f0c5af3db9b0159d9b1a89faff3bd047510b628

llvm/include/llvm/MC/MCWinCOFFStreamer.h

public:
   void emitLocalCommonSymbol(MCSymbol *Symbol, uint64_t Size,
                              unsigned ByteAlignment) override;
   void emitZerofill(MCSection *Section, MCSymbol *Symbol, uint64_t Size,
                     unsigned ByteAlignment, SMLoc Loc = SMLoc()) override;
   void emitTBSSSymbol(MCSection *Section, MCSymbol *Symbol, uint64_t Size,
                       unsigned ByteAlignment) override;
   void emitIdent(StringRef IdentString) override;
   void EmitWinEHHandlerData(SMLoc Loc) override;
+  void emitCGProfileEntry(const MCSymbolRefExpr *From,
+                          const MCSymbolRefExpr *To, uint64_t Count) override;
   void finishImpl() override;
 
   /// \}
 
 protected:
   const MCSymbol *CurSymbol;
 
   void emitInstToData(const MCInst &Inst, const MCSubtargetInfo &STI) override;
 
+  void finalizeCGProfileEntry(const MCSymbolRefExpr *&S);
+  void finalizeCGProfile();
+
 private:
   void Error(const Twine &Msg) const;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_MC_MCWINCOFFSTREAMER_H

llvm/lib/MC/MCParser/COFFAsmParser.cpp

void Initialize(MCAsmParser &Parser) override {
     addDirectiveHandler<&COFFAsmParser::ParseDirectiveEndef>(".endef");
     addDirectiveHandler<&COFFAsmParser::ParseDirectiveSecRel32>(".secrel32");
     addDirectiveHandler<&COFFAsmParser::ParseDirectiveSymIdx>(".symidx");
     addDirectiveHandler<&COFFAsmParser::ParseDirectiveSafeSEH>(".safeseh");
     addDirectiveHandler<&COFFAsmParser::ParseDirectiveSecIdx>(".secidx");
     addDirectiveHandler<&COFFAsmParser::ParseDirectiveLinkOnce>(".linkonce");
     addDirectiveHandler<&COFFAsmParser::ParseDirectiveRVA>(".rva");
     addDirectiveHandler<&COFFAsmParser::ParseDirectiveSymbolAttribute>(".weak");
+    addDirectiveHandler<&COFFAsmParser::ParseDirectiveCGProfile>(".cg_profile");
 
     // Win64 EH directives.
     addDirectiveHandler<&COFFAsmParser::ParseSEHDirectiveStartProc>(
                                                                    ".seh_proc");
     addDirectiveHandler<&COFFAsmParser::ParseSEHDirectiveEndProc>(
                                                                 ".seh_endproc");
     addDirectiveHandler<&COFFAsmParser::ParseSEHDirectiveStartChained>(
                                                            ".seh_startchained");
class COFFAsmParser : public MCAsmParserExtension {
   bool ParseDirectiveEndef(StringRef, SMLoc);
   bool ParseDirectiveSecRel32(StringRef, SMLoc);
   bool ParseDirectiveSecIdx(StringRef, SMLoc);
   bool ParseDirectiveSafeSEH(StringRef, SMLoc);
   bool ParseDirectiveSymIdx(StringRef, SMLoc);
   bool parseCOMDATType(COFF::COMDATType &Type);
   bool ParseDirectiveLinkOnce(StringRef, SMLoc);
   bool ParseDirectiveRVA(StringRef, SMLoc);
+  bool ParseDirectiveCGProfile(StringRef, SMLoc);
 
   // Win64 EH directives.
   bool ParseSEHDirectiveStartProc(StringRef, SMLoc);
   bool ParseSEHDirectiveEndProc(StringRef, SMLoc);
   bool ParseSEHDirectiveStartChained(StringRef, SMLoc);
   bool ParseSEHDirectiveEndChained(StringRef, SMLoc);
   bool ParseSEHDirectiveHandler(StringRef, SMLoc);
   bool ParseSEHDirectiveHandlerData(StringRef, SMLoc);
while (true) {
       Lex();
     }
   }
 
   Lex();
   return false;
 }
 
+/// ParseDirectiveCGProfile
+///  ::= .cg_profile identifier, identifier, <number>
+bool COFFAsmParser::ParseDirectiveCGProfile(StringRef, SMLoc) {
+  StringRef From;
+  SMLoc FromLoc = getLexer().getLoc();
+  if (getParser().parseIdentifier(From))
+    return TokError("expected identifier in directive");
+
+  if (getLexer().isNot(AsmToken::Comma))
+    return TokError("expected a comma");
+  Lex();
+
+  StringRef To;
+  SMLoc ToLoc = getLexer().getLoc();
+  if (getParser().parseIdentifier(To))
+    return TokError("expected identifier in directive");
+
+  if (getLexer().isNot(AsmToken::Comma))
+    return TokError("expected a comma");
+  Lex();
+
+  int64_t Count;
+  if (getParser().parseIntToken(
+          Count, "expected integer count in '.cg_profile' directive"))
+    return true;
+
+  if (getLexer().isNot(AsmToken::EndOfStatement))
+    return TokError("unexpected token in directive");
+
+  MCSymbol *FromSym = getContext().getOrCreateSymbol(From);
+  MCSymbol *ToSym = getContext().getOrCreateSymbol(To);
+
+  // Only store non-temporary symbols.
+  if (!FromSym->isTemporary() && !ToSym->isTemporary())
+    getStreamer().emitCGProfileEntry(
+        MCSymbolRefExpr::create(FromSym, MCSymbolRefExpr::VK_None, getContext(),
+                                FromLoc),
+        MCSymbolRefExpr::create(ToSym, MCSymbolRefExpr::VK_None, getContext(),
+                                ToLoc),
+        Count);
+  return false;
+}
+
 bool COFFAsmParser::ParseSectionSwitch(StringRef Section,
                                        unsigned Characteristics,
                                        SectionKind Kind) {
   return ParseSectionSwitch(Section, Characteristics, Kind, "", (COFF::COMDATType)0);
 }
 
 bool COFFAsmParser::ParseSectionSwitch(StringRef Section,
                                        unsigned Characteristics,

llvm/lib/MC/MCWinCOFFStreamer.cpp

 void MCWinCOFFStreamer::emitIdent(StringRef IdentString) {
   llvm_unreachable("not implemented");
 }
 
 void MCWinCOFFStreamer::EmitWinEHHandlerData(SMLoc Loc) {
   llvm_unreachable("not implemented");
 }
 
+void MCWinCOFFStreamer::emitCGProfileEntry(const MCSymbolRefExpr *From,
+                                           const MCSymbolRefExpr *To,
+                                           uint64_t Count) {
+  getAssembler().CGProfile.push_back({From, To, Count});
+}
+
+void MCWinCOFFStreamer::finalizeCGProfileEntry(const MCSymbolRefExpr *&SRE) {
+  const MCSymbol *S = &SRE->getSymbol();
+  bool Created;
+  getAssembler().registerSymbol(*S, &Created);
+  if (Created) {
+    cast<MCSymbolCOFF>(S)->setIsWeakExternal();
+    cast<MCSymbolCOFF>(S)->setExternal(true);
+  }
+}
+
+void MCWinCOFFStreamer::finalizeCGProfile() {
+  for (MCAssembler::CGProfileEntry &E : getAssembler().CGProfile) {
+    finalizeCGProfileEntry(E.From);
+    finalizeCGProfileEntry(E.To);
+  }
+}
+
 void MCWinCOFFStreamer::finishImpl() {
+  finalizeCGProfile();
+
   MCObjectStreamer::finishImpl();
 }
 
 void MCWinCOFFStreamer::Error(const Twine &Msg) const {
   getContext().reportError(SMLoc(), Msg);
 }

llvm/lib/MC/WinCOFFObjectWriter.cpp

public:
   symbol_list WeakDefaults;
 
   bool UseBigObj;
 
   bool EmitAddrsigSection = false;
   MCSectionCOFF *AddrsigSection;
   std::vector<const MCSymbol *> AddrsigSyms;
 
+  MCSectionCOFF *CGProfileSection = nullptr;
+
   WinCOFFObjectWriter(std::unique_ptr<MCWinCOFFObjectTargetWriter> MOTW,
                       raw_pwrite_stream &OS);
 
   void reset() override {
     memset(&Header, 0, sizeof(Header));
     Header.Machine = TargetObjectWriter->getMachine();
     Sections.clear();
     Symbols.clear();
void WinCOFFObjectWriter::executePostLayoutBinding(MCAssembler &Asm,
                                                    const MCAsmLayout &Layout) {
   if (EmitAddrsigSection) {
     AddrsigSection = Asm.getContext().getCOFFSection(
         ".llvm_addrsig", COFF::IMAGE_SCN_LNK_REMOVE,
         SectionKind::getMetadata());
     Asm.registerSection(*AddrsigSection);
   }
 
+  if (!Asm.CGProfile.empty()) {
+    CGProfileSection = Asm.getContext().getCOFFSection(
+        ".llvm.call-graph-profile", COFF::IMAGE_SCN_LNK_REMOVE,
+        SectionKind::getMetadata());
+    Asm.registerSection(*CGProfileSection);
+  }
+
   // "Define" each section & symbol. This creates section & symbol
   // entries in the staging area.
   for (const auto &Section : Asm)
     defineSection(static_cast<const MCSectionCOFF &>(Section));
 
   for (const MCSymbol &Symbol : Asm.symbols())
     if (!Symbol.isTemporary())
       DefineSymbol(Symbol, Asm, Layout);
for (const MCSymbol *S : AddrsigSyms) {
       MCSection *TargetSection = &S->getSection();
       assert(SectionMap.find(TargetSection) != SectionMap.end() &&
              "Section must already have been defined in "
              "executePostLayoutBinding!");
       encodeULEB128(SectionMap[TargetSection]->Symbol->getIndex(), OS);
     }
   }
 
+  // Create the contents of the .llvm.call-graph-profile section.
+  if (CGProfileSection) {
+    auto *Frag = new MCDataFragment(CGProfileSection);
+    Frag->setLayoutOrder(0);
+    raw_svector_ostream OS(Frag->getContents());
+    for (const MCAssembler::CGProfileEntry &CGPE : Asm.CGProfile) {
+      uint32_t FromIndex = CGPE.From->getSymbol().getIndex();
+      uint32_t ToIndex = CGPE.To->getSymbol().getIndex();
+      OS.write((const char *)&FromIndex, sizeof(uint32_t));
+      OS.write((const char *)&ToIndex, sizeof(uint32_t));
+      OS.write((const char *)&CGPE.Count, sizeof(uint64_t));
+    }
+  }
+
   assignFileOffsets(Asm, Layout);
 
   // MS LINK expects to be able to use this timestamp to implement their
   // /INCREMENTAL feature.
   if (Asm.isIncrementalLinkerCompatible()) {
     Header.TimeDateStamp = getTime();
   } else {
     // Have deterministic output if /INCREMENTAL isn't needed. Also matches GNU.

llvm/test/MC/AsmParser/directive_cgprofile.s

 # RUN: llvm-mc -triple i386-unknown-unknown %s | FileCheck %s
-
+# RUN: llvm-mc -triple x86_64-pc-win32 %s | FileCheck %s
   .cg_profile a, b, 32
   .cg_profile freq, a, 11
   .cg_profile freq, b, 20
 
 # CHECK: .cg_profile a, b, 32
 # CHECK: .cg_profile freq, a, 11
 # CHECK: .cg_profile freq, b, 20

llvm/test/MC/COFF/cgprofile.s

+# RUN: llvm-mc -filetype=obj -triple x86_64-pc-win32 %s -o - | llvm-readobj -S --symbols --sd --cg-profile | FileCheck %s
+
+  .section .test,"w"
+a:
+
+  .cg_profile a, b, 32
+  .cg_profile freq, a, 11
+  .cg_profile late, late2, 20
+  .cg_profile .L.local, b, 42
+
+  .globl late
+late:
+late2: .word 0
+late3:
+.L.local:
+
+# CHECK:      Name: .llvm.call-graph-profile
+# CHECK-NEXT: VirtualSize:
+# CHECK-NEXT: VirtualAddress:
+# CHECK-NEXT: RawDataSize: 48
+# CHECK-NEXT: PointerToRawData:
+# CHECK-NEXT: PointerToRelocations:
+# CHECK-NEXT: PointerToLineNumbers:
+# CHECK-NEXT: RelocationCount:
+# CHECK-NEXT: LineNumberCount:
+# CHECK-NEXT: Characteristics [ (0x100800)
+# CHECK-NEXT:   IMAGE_SCN_ALIGN_1BYTES (0x100000)
+# CHECK-NEXT:   IMAGE_SCN_LNK_REMOVE (0x800)
+# CHECK-NEXT: ]
+# CHECK-NEXT: SectionData (
+# CHECK-NEXT:   0000: 0A000000 0E000000 20000000 00000000
+# CHECK-NEXT:   0010: 11000000 0A000000 0B000000 00000000
+# CHECK-NEXT:   0020: 0B000000 0C000000 14000000 00000000
+# CHECK-NEXT: )
+
+# CHECK: Symbols [
+# CHECK:      Name: a
+# CHECK-NEXT: Value:
+# CHECK-NEXT: Section: .test
+# CHECK-NEXT: BaseType:
+# CHECK-NEXT: ComplexType:
+# CHECK-NEXT: StorageClass: Static
+# CHECK-NEXT: AuxSymbolCount:
+# CHECK:      Name: late
+# CHECK-NEXT: Value:
+# CHECK-NEXT: Section: .test
+# CHECK-NEXT: BaseType:
+# CHECK-NEXT: ComplexType:
+# CHECK-NEXT: StorageClass: External
+# CHECK-NEXT: AuxSymbolCount:
+# CHECK:      Name: late2
+# CHECK-NEXT: Value:
+# CHECK-NEXT: Section: .test
+# CHECK-NEXT: BaseType:
+# CHECK-NEXT: ComplexType:
+# CHECK-NEXT: StorageClass: Static
+# CHECK-NEXT: AuxSymbolCount:
+# CHECK:      Name: late3
+# CHECK-NEXT: Value:
+# CHECK-NEXT: Section: .test
+# CHECK-NEXT: BaseType:
+# CHECK-NEXT: ComplexType:
+# CHECK-NEXT: StorageClass: Static
+# CHECK-NEXT: AuxSymbolCount:
+# CHECK:      Name: b
+# CHECK-NEXT: Value:
+# CHECK-NEXT: Section: IMAGE_SYM_UNDEFINED
+# CHECK-NEXT: BaseType:
+# CHECK-NEXT: ComplexType:
+# CHECK-NEXT: StorageClass: WeakExternal
+# CHECK-NEXT: AuxSymbolCount: 1
+# CHECK-NEXT: AuxWeakExternal {
+# CHECK-NEXT:   Linked: .weak.b.default.late
+# CHECK-NEXT:   Search: Alias
+# CHECK-NEXT: }
+# CHECK:      Name: .weak.b.default.late
+# CHECK-NEXT: Value:
+# CHECK-NEXT: Section: IMAGE_SYM_ABSOLUTE
+# CHECK-NEXT: BaseType:
+# CHECK-NEXT: ComplexType:
+# CHECK-NEXT: StorageClass: External
+# CHECK-NEXT: AuxSymbolCount: 0
+# CHECK:      Name: freq
+# CHECK-NEXT: Value:
+# CHECK-NEXT: Section: IMAGE_SYM_UNDEFINED
+# CHECK-NEXT: BaseType:
+# CHECK-NEXT: ComplexType:
+# CHECK-NEXT: StorageClass: WeakExternal
+# CHECK-NEXT: AuxSymbolCount: 1
+# CHECK-NEXT: AuxWeakExternal {
+# CHECK-NEXT:   Linked: .weak.freq.default.late
+# CHECK-NEXT:   Search: Alias
+# CHECK-NEXT: }
+# CHECK:      Name: .weak.freq.default.late
+# CHECK-NEXT: Value:
+# CHECK-NEXT: Section: IMAGE_SYM_ABSOLUTE
+# CHECK-NEXT: BaseType:
+# CHECK-NEXT: ComplexType:
+# CHECK-NEXT: StorageClass: External
+# CHECK-NEXT: AuxSymbolCount: 0
+
+# CHECK:      CGProfile [
+# CHECK-NEXT:   CGProfileEntry {
+# CHECK-NEXT:     From: a
+# CHECK-NEXT:     To: b
+# CHECK-NEXT:     Weight: 32
+# CHECK-NEXT:   }
+# CHECK-NEXT:   CGProfileEntry {
+# CHECK-NEXT:     From: freq
+# CHECK-NEXT:     To: a
+# CHECK-NEXT:     Weight: 11
+# CHECK-NEXT:   }
+# CHECK-NEXT:   CGProfileEntry {
+# CHECK-NEXT:     From: late
+# CHECK-NEXT:     To: late2
+# CHECK-NEXT:     Weight: 20
+# CHECK-NEXT:   }
+# CHECK-NEXT: ]
 No newline at end of file