[PATCH] D105917: [llvm-objdump][CallGraphSection] Extract call graph information from binary

Wed Jul 28 22:08:37 PDT 2021

necipfazil marked 2 inline comments as done.
necipfazil added a comment.

In D105917#2903706 <https://reviews.llvm.org/D105917#2903706>, @jhenderson wrote:

> 

Thanks for your feedback.

> Please don't commit this yet. there's a lot of code, and I'd like to spend quite some time reviewing it - a first glance through raises some initial concerns, but I need to understand the whole thing to be able to properly consider them and possible suggestions.
>
> Some initial high-level comments/questions:
>
> 1. Is it possible to break this patch down into smaller parts, perhaps implementing some subset of the functionality per part? That'll make things easier to review.

This patch is not split to 6 smaller patches: D107028 <https://reviews.llvm.org/D107028>, D107029 <https://reviews.llvm.org/D107029>, D107030 <https://reviews.llvm.org/D107030>, D107031 <https://reviews.llvm.org/D107031>, D107032 <https://reviews.llvm.org/D107032>, D107033 <https://reviews.llvm.org/D107033>. We can now abandon this revision.

> 2. Why is this functionality being added to llvm-objdump rather than llvm-readobj? (There may be a perfectly valid reason to do so, but llvm-readobj tends to be the place we put things to interpret arbitrary sections).

We would like to dissasemble the object for information on functions and call sites. The first attempt was to use llvm-readobj but llvm-objdump seems to provide a large amount of code reuse for our case. Especially see D107029 <https://reviews.llvm.org/D107029> for functions and D107030 <https://reviews.llvm.org/D107030> for call sites.

> 3. Can we avoid the canned binary in the test, please? Use an input generated at runtime using either llvm-mc or yaml2obj (the latter is preferable, but likely requires adding functionality to yaml2obj for the new section type).

I added a bunch of additional (and smaller) llvm-mc/yaml2obj tests with the new set of patches.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105917