[PATCH] D115973: [llvm-profgen] Support symbol loading for debug fission

Mon Feb 14 13:50:31 PST 2022

wlei added a comment.

In D115973#3320536 <https://reviews.llvm.org/D115973#3320536>, @MaskRay wrote:

> Did you read jhenderson's point?
>
>> 1. yaml2obj with DWARF, if the support is sufficiently mature for your use-case, and the output is easy enough to read - one advantage with DWARF yaml2obj is that you can omit many of the required fields, unlike assembly, so depending on the complexity you need, this may be a better option for long-term maintenance.
>
> The tests are clumsy. I don't think we should land the change as is. 
> `split-dwarf-split.exe.yaml` has nearly 800 lines and includes many tags which are not needed for the purpose of the test. For example, all the STT_SECTION symbols, .eh_frame, .init_array, and .dynsym can be deleted.
> The `.debug_*` sections are opaque and difficult to maintain.
> If the readability of `split-dwarf-split.exe.yaml` cannot be improved after some cleanups, try leaving a small stub test and consider adding the fullblown test (simplification still required) in cross-project-tests.

@MaskRay Thanks for your feedbacks!  I did try to reduce the `*.exe.yaml` file but found that it's hard to do this because there're a lot of dependences, I think it's also less productive for people in the future to reproduce that.

> The `.debug_*` sections are opaque and difficult to maintain.

Yeah, but the test is tested against those debug sections, I guess using assembly should have the same issue. Considering this, it seems the only way to leave for cross-project-tests?

> try leaving a small stub test and consider adding the fullblown test (simplification still required) in cross-project-tests.

Could you elaborate more on this? The exe binary is the root dependence of those tests. Regarding the stub test, do you mean leave a .c source file in llvm/test/.., then move the whole test file into cross-project-tests?

I did also play with the cross-project-tests, but it seems it isolates the test from the llvm tool(llvm-profgen here). Before we can just `ninja check-llvm-tools`, but now we need to run more cmds. You see, in llvm-symbolizer test https://github.com/llvm/llvm-project/tree/main/llvm/test/tools/llvm-symbolizer/Inputs, there're still many exe for split dwarf. This is exactly the same to this patch, I'm curious is there any concerns not moving llvm-symbolizer test into cross-project-tests? cc  @jhenderson @dblaikie

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115973