[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC

[Bob Haarman via Phabricator via lldb-commits]

inglorion added a comment.

I also think we should revert this change while we think about the approach. I've been reluctant to do this because I was impressed by how many bytes of output it saves, and I didn't want to lose that just because it doesn't play nice with ThinLTO. However, after some more data gathering and talking to people, I think that:

1. There is enough doubt that this is the right approach that we probably should take some time to think about which way we really want to go.

2. In the meantime, a build configuration that used to work is broken.

3. To fix that while leaving this change in, we would have to build more things on top of the change, which seems unwise if we're not sure that this is the best approach.

So I think we should revert and rethink.

Reasons I'm not convinced that this is the right approach:

1. We have now identified multiple use cases that break with this change. ThinLTO is one, -fmodules-debuginfo is another. The general theme here is that the expectation is that if an object file is passed to the linker and not between --start-lib and --end-lib or in a static library, the expectation is that it makes it into the final link. -gc-sections asks to discard unused sections, but it's not clear that this should also apply to debug information, and we have seen some problems.

2. It's also not clear to me that this is as much of a win as I initially thought. It is in the cases that have been demonstrated, of course, but I'm not sure how well that generalizes. On a local Clang build I performed, for example, it made exactly 0 bytes difference. Perhaps the big wins only happen in pathological cases that are better addressed some other way.

So I'm going to revert this to unbreak the Chromium/Android build, and then we can think some more about how we can get the size win without the breakage.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54747