[PATCH] D44965: [llvm][Instrumentation/MC] Add Call Graph Profile pass and object file emission.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: docs/Extensions.rst:326
+If this is the first usage of either of the symbols then that symbol is declared
+as if ``.weak symbol`` had been written.  This forces the symbol to show up in
+the symbol table.
----------------
This seems a little weird. In addition to the cases you mentioned, it would also seem to imply that code like this:
```
.cg_profile foo, bar, 42

.globl foo
foo:
call bar
```
will cause `bar` to become `STB_WEAK`. I guess the idea behind setting the binding to `STB_WEAK`rather than `STB_GLOBAL` is that you don't want to cause an undefined symbol error in the case where all other object files use `STB_WEAK` for the symbol and one of those object files contains a relocation to the symbol? In that case, maybe we should only end up setting the binding to weak if we don't emit any relocations for the symbol (i.e. `isUsedInReloc()` is false)?


================
Comment at: lib/MC/ELFObjectWriter.cpp:987
+
+  case ELF::SHT_LLVM_CALL_GRAPH_PROFILE:
+    sh_link = SymbolTableIndex;
----------------
You can move this case under `SHT_SYMTAB_SHNDX`.


================
Comment at: test/MC/ELF/cgprofile.s:9
+    .cg_profile late, late2, 20
+	.cg_profile .L.local, b, 42
+
----------------
Using a symbol index of 0 seems like valid behaviour, but maybe it would be a little better to point to the section symbol instead?


https://reviews.llvm.org/D44965