[PATCH] D96931: [LTO] Discard non-prevailing defined symbols in module-level assembly

Fri Feb 26 09:34:10 PST 2021

tejohnson added a comment.

In D96931#2575999 <https://reviews.llvm.org/D96931#2575999>, @ychen wrote:

> Thanks for the comment!
>
> In D96931#2574711 <https://reviews.llvm.org/D96931#2574711>, @tejohnson wrote:
>
>> I don't love the custom pattern matching approach added here - it is hard to follow and seems potentially brittle. If this case is rare, presumably it is rare that we would have a non-empty set of non prevailing asm symbols, so using something heavier weight shouldn't be an issue if that would work and be clearer.
>
> Me neither. One benefit though is that only one place in the source code needs inspection if anything goes off the rail and it doesn't *pollute* other parts of the LTO code. The AsmParser & AsmStreamer solution would (1) involve much more change in LTO/ModuleSymbolTable/MCAsmStreamer (need to create derived class from MCAsmStreamer which is not exposed now) (2) there would be inevitable code sharing/refactoring with `ModuleSymbolTable`. I really don't think it is a good idea even if it does not impact compile time.
>
> After some thought, I think a third approach is possible: emit the list of to-be-discarded symbol names to a module metadata node and discard these symbols during the normal `AsmPrinter::emitInlineAsm` process. This should have a minimal compile-time impact, easy implementation. A fourth approach would be to demote the error introduced by D90108 <https://reviews.llvm.org/D90108> to a warning.
>
> Please let me know which approach is preferred.
>
>> There are no comments right now so it isn't clear to me what this is doing. What is the behavior before and after the patch? Is this patch fixing an issue created by LTO, or attempting to fix bad input?
>
> Before this patch, the test fails with "foo changed binding to STB_GLOBAL" because, in the combined LTO module, there is conceptually `.weak foo; .global foo` hence triggers the error diagnose introduced by D90108 <https://reviews.llvm.org/D90108>. This patch discards the `.weak foo` part of `.weak foo; .global foo` to avoid the error diagnosis. I think it is a borderline LTO issue in that we don't handle the external symbols in module asm block with existing symbol resolutions. Although most of the time (including the test case in this patch), the behavior happens to be correct. ThinLTO and non-LTO do not suffer this issue because two bindings (`.weak` and `.global`) end up in two object files, the error diagnosis does not trigger then.

Thanks for explaining the situation better. Ideally LTO or MC would mimic the behavior of the linker, which I think would actually be to take the strong def regardless of linking order. When the IR is read and the IRSymbolTable created, the asm symbols are parsed and added so the linker would resolve them. However, I don't have a good idea how to use that when merging the module inline asm together.

@pcc any suggestions?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96931