[PATCH] D128863: Add switch to use "source_file_name" instead of Module ID for globally promoted local

[Bill Wendling via Phabricator via llvm-commits]

void added a comment.

In D128863#3660913 <https://reviews.llvm.org/D128863#3660913>, @tejohnson wrote:

> In D128863#3660850 <https://reviews.llvm.org/D128863#3660850>, @void wrote:
>
>> In D128863#3660840 <https://reviews.llvm.org/D128863#3660840>, @tejohnson wrote:
>>
>>> In D128863#3660585 <https://reviews.llvm.org/D128863#3660585>, @void wrote:
>>>
>>>> In D128863#3659642 <https://reviews.llvm.org/D128863#3659642>, @tejohnson wrote:
>>>>
>>>>> Ok, I understand the issue. The hash changes due to code changes in a module A that is allowed to change, but this has the side effect of superficially changing the callsite in another module B that isn't allowed to change. Couple concerns below.
>>>>>
>>>>> Fundamentally, LTO is doing whole program and cross-module optimization. How do you ensure that a source change to module A doesn't actually affect the optimization in module B? E.g. the reason you are getting a call to a local foo from module A in module B in the first place is that module B decided to import the caller of foo. If foo or its caller changed in module A the importing decisions into module B could actually change its code generation. We could decide not to import foo's caller or foo itself might get imported. There are other whole program optimizations that could also affect the code generation in B depending on changes in other modules. How will you handle or prevent these cases?
>>>>
>>>> The short answer is "we won't be able to prevent this." It's one of the risks of using LTO with live-patching (a.k.a. ksplice, kpatch, klivepatch). The risk itself isn't that it will change other places where it was inlined (that's expected, of course), but that the patch may become much larger, making it more difficult to validate. If the patch remains of reasonable size, then the risk involved in using said patch will be minimal.
>>>
>>> If you can handle a change in module A causing a change in module B due to different cross-module inlining by making the patch larger (and I guess including both modules), could you handle this case, where there is a rename of a function in module A causing the callsite in module B to be renamed, similarly?
>>
>> That's not quite the situation.
>>
>> Let's say I have module `A` and I patch function `foo()` and get module `B`. A local function in `A`, named `bar()`, is promoted during its compilation to `bar.llvm.1234()`. After patching, `B` will also promote `bar()` to a global, but give it a different suffix, say `bar.llvm.abcd()`. A separate tool comes along to determine what's changed between the `A`'s ELF file and `B`'s ELF file. It sees that `bar.llvm.1234()` is now `bar.llvm.abcd()` and claims that's a change. (In the instance that came up, the changed function was called from a section we forbid changes in.)
>
> Sure, I believe I understand what's happening in your situation. But I think you and I are using the terms "module A" and "module B" differently. Here your module B is the new version of module A after patching. I was referring to 2 modules from totally different source files. What I was thinking of is a case where there are 2 source files A.cpp and B.cpp that become modules A and B:
>
> A.cpp
> static void bar() {...}
> void foo() { bar(); }
>
> B.cpp
> void caller() { foo(); }
>
> I was assuming you had a situation where with ThinLTO we had imported and inlined foo into module B, requiring bar to be promoted in module A, say to bar.llvm.1 (and leading to a call to the promoted bar.llvm.1 in module B). Then if something in A.cpp got patched, let's just say bar itself got patched, then you build a new module A object where bar is promoted with a different suffix, say to bar.llvm.2. And now you have changes to both modules A and B.
>
> But you could just as easily change the importing decisions into module B due to a change to bar. E.g. if bar was made smaller, we might decide to import and inline it as well into module B. It sounds like you would handle this situation by including the objects for both module A and B in the patch. I was just wondering if the former case could be handled similarly. I.e. include the new objects for both modules A and B in the patch since there were changes to both as a result of the change in A.cpp.
>
> If the change is in a section that isn't allowed to be patched, then that seems like it could cause an issue for the other example I gave above too (where we decided to go ahead and import/inline bar as well). Is that correct?

Yes, that could happen, but it's a separate issue than what I'm trying to solve here. :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128863