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

[Michael Spencer via Phabricator via llvm-commits]

Bigcheese marked an inline comment as done.
Bigcheese 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.
----------------
pcc wrote:
> 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)?
That would require delaying processing of .cg_profile entries until the end, but given the above issue, that's probably worth it.  Another option is to error if the symbol doesn't already exist.


================
Comment at: test/MC/ELF/cgprofile.s:9
+    .cg_profile late, late2, 20
+	.cg_profile .L.local, b, 42
+
----------------
pcc wrote:
> 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?
Yeah, that would match the standard behavior of .L.


https://reviews.llvm.org/D44965