[PATCH] D105916: [AsmPrinter][CallGraphSection] Emit call graph section

[Fangrui Song via Phabricator via llvm-commits]

MaskRay requested changes to this revision.
MaskRay added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:204
+
+  DenseMap<Function *, FunctionInfo> FunctionLabels;
+
----------------
necipfazil wrote:
> MaskRay wrote:
> > unstable iteration order. may consider MapVector https://llvm.org/docs/ProgrammersManual.html#llvm-adt-mapvector-h
> We iterate over `FunctionLabels` only once when emitting the labels, and do not mutate the map while so.
> 
> If there is no other specific reason, we may keep using `DenseMap` as there is no need for emitting the functions in a certain order.
> there is no need for emitting the functions in a certain order.

The compiler must enforce an order for reproducible builds. Non-determinism is bad for build cache.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1293
+    enum FunctionKind {
+      NOT_INDIRECT_TARGET = 0,
+      INDIRECT_TARGET_UNKNOWN_TID = 1,
----------------
Move this outside and make it clear that the section consumer needs to synchronize with the values.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1327
+      // Emit type id.
+      OutStreamer->emitInt64(TypeId);
+
----------------
Perhaps consider a compact style where you place comments on the right side of the code.


================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:1046
+                            GroupName, true, ElfSec.getUniqueID(),
+                            cast<MCSymbolELF>(FunctionSym));
+}
----------------
getStackSizesSection uses getBeginSYmbol on TextSec.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105916