This is an archive of the discontinued LLVM Phabricator instance.

[IRMover] Remove UB implying parameter attributes when necessary
AcceptedPublic

Authored by TimNN on Dec 2 2022, 10:05 AM.

Details

Summary

When importing functions from some module X into some module Y, they may reference other functions already present in Y. The signature (especially attributes) of those other functions may have diverged between X and Y (e.g. due to the Dead Argument Elimination optimization). If necessary, modify the attributes to avoid UB.

See the added test and implementation comments for more details.

This was exposed by https://reviews.llvm.org/D133036 before it was reverted. Fixes https://github.com/llvm/llvm-project/issues/58976.

Diff Detail

Event Timeline

TimNN created this revision.Dec 2 2022, 10:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2022, 10:05 AM
TimNN published this revision for review.Dec 2 2022, 10:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2022, 10:08 AM
tejohnson added inline comments.Dec 2 2022, 10:54 AM
llvm/lib/Linker/IRMover.cpp
614

It seems this can/should only be an issue when the dest module only contains a declaration, and we are only importing a declaration. Should we be checking that F->isDeclaration() (or doing it after that check falls through)? And should we further check that we wouldn't invoke linkGlobalValueBody() further below? Wondering if the change to ensure function attr compatibility is either unnecessary or would be an error in either of those cases.

llvm/test/Transforms/FunctionImport/attr_fixup.ll
11 ↗(On Diff #479669)

Suggest making Inputs/attr_fixup.ll contain the post DAE code, rather than running that and expecting it will make the necessary change.

TimNN added inline comments.Dec 5 2022, 2:09 PM
llvm/lib/Linker/IRMover.cpp
614

Doing this only when both are declarations makes sense, I think. (Either way, I'll move this to a separate if statement because all the cases here handle the "If we already created the body, just return." case).

While thinking about this I found an (IMO) more severe issue. I was initially thinking that definitions with internal linkage may need to get their attributes updated. Then I realized that reusing an existing symbol with internal linkage when importing a function was probably always wrong (because they could have completely different definitions).

When I experimented with this, I found that the current code actually _does_ (incorrectly, IIUC) reuse symbols with internal linkage, see https://github.com/llvm/llvm-project/issues/59347 for a repro. I feel like that should be fixed first before continuing this patch.

tejohnson added inline comments.Dec 6 2022, 7:17 AM
llvm/lib/Linker/IRMover.cpp
614

We promote to global scope and rename any local functions that are either exported by reference or definition to another module. I can see why that didn't happen with the test case you created, as you didn't do any post thin-link updates on the other module. Will make a comment on the github issue.

TimNN updated this revision to Diff 480592.Dec 6 2022, 1:11 PM

Address review comments

  • Move the fixup to after the existing if.
  • Only perform the fixup when the source is a definition and the destination a declaration.
  • Change the test to contain the post-deadargelim IR.
TimNN marked 3 inline comments as done.Dec 6 2022, 1:16 PM

Thanks for the review and also your input on https://github.com/llvm/llvm-project/issues/59347!

llvm/lib/Linker/IRMover.cpp
614

I'm not sure about linkGlobalValueBody(), I left things as-is with regard to that. Let me know if you want me to take this into account.

TimNN updated this revision to Diff 480805.Dec 7 2022, 1:17 AM

Sync to HEAD to check if that resolves the (IMO likely unrelated) test timeout.

tejohnson added inline comments.Dec 12 2022, 1:45 PM
llvm/lib/Linker/IRMover.cpp
606

I thought this could only happen if Src's definition is not linked in.

Ah, Src may contain a def but we may not link in that def (depending on whether we decide to invoke linkGlobalValueBody). I believe if the def of Src was linked in, we wouldn't have an issue, since presumably that def would contain the correct attributes? I assume if you remove the "noinline" from inner in the Input file for the test it's def gets linked in - I forget, does that work correctly?

Assuming the answer to the last question above is yes, then I think you would only want to call this when we don't invoke linkGlobalValueBody.

Then I don't think you need or want this if statement at all (we've already checked in the caller that dest is a declaration).

TimNN updated this revision to Diff 482578.Dec 13 2022, 11:49 AM

Only call ensureFunctionCompatibility if linkGlobalValueBody isn't called.

TimNN marked an inline comment as done.Dec 13 2022, 11:50 AM
TimNN added inline comments.
llvm/lib/Linker/IRMover.cpp
606

Ack, makes sense, I think I understand this code a bit better now. Removed this if statement and change the code to only call ensureFunctionCompatibility if the body did not get linked.

And yes, removing the noinline fixes the problem (though the original issues happened without noinline, i.e. the optimizier decided to not inline @inner for some other reason).

tejohnson added a subscriber: gulfem.

Sorry for the delay, I had another review of attribute updates come in (D137360), can you work with @gulfem to have a consistent place to do these updates? I'm thinking that there could be a single "updateAttributes" function, and in there you could invoke your ensureFunctionCompatibility if New is still a declaration (see D137360, I've suggested removeAttributes be renamed to updateAttributes).

Sorry for the delay, I had another review of attribute updates come in (D137360), can you work with @gulfem to have a consistent place to do these updates? I'm thinking that there could be a single "updateAttributes" function, and in there you could invoke your ensureFunctionCompatibility if New is still a declaration (see D137360, I've suggested removeAttributes be renamed to updateAttributes).

@TimNN,
Based on @tejohnson 's suggestion, I renamed the function to updateAttributes in D137360. How would you like to coordinate? I'm thinking that I can land D137360, and you can extend updateAttributes() function to include a src and dest arguments, and call your ensureFunctionCompatibility() function from there. Does that sound good or do you have another suggestion?

TimNN marked an inline comment as done.Dec 22 2022, 12:55 PM

I'm thinking that I can land D137360, and you can extend updateAttributes() function to include a src and dest arguments, and call your ensureFunctionCompatibility() function from there.

Yeah, sounds good! I don't know when I'll have time to come back to this during the holidays, so go ahead and submit yours if is ready and I'll adapt.

I'm thinking that I can land D137360, and you can extend updateAttributes() function to include a src and dest arguments, and call your ensureFunctionCompatibility() function from there.

Yeah, sounds good! I don't know when I'll have time to come back to this during the holidays, so go ahead and submit yours if is ready and I'll adapt.

Great, I landed D137360! Feel free to change updateAttributes() function when you revisit this. Happy holidays!

TimNN updated this revision to Diff 488941.Jan 13 2023, 4:20 AM

Adapt to changes from D137360.

This reverts to not taking into account whether linkGlobalValueBody was called because that information is not readily available in the helper added by D137360 and I did not want to proxy that information via another parameter.

TimNN edited the summary of this revision. (Show Details)Jan 13 2023, 4:21 AM
arsenm added a subscriber: arsenm.Jan 13 2023, 5:19 AM
arsenm added inline comments.
llvm/lib/Linker/IRMover.cpp
1561

Drive by comment, but is this brokenly not checking for CallBase?

llvm/test/Transforms/FunctionImport/Inputs/attr_fixup_dae_noundef.ll
7

Shouldn't use anonymous values in tests

arsenm added inline comments.Jan 13 2023, 5:20 AM
llvm/lib/Linker/IRMover.cpp
1577–1579

I thought we casted the function type at callsites for different signatures in different modules, so this wouldn't hold?

TimNN updated this revision to Diff 488990.Jan 13 2023, 6:58 AM
TimNN marked an inline comment as done.

Don't use anonymous values in tests

TimNN marked an inline comment as done.Jan 13 2023, 7:00 AM
TimNN added inline comments.
llvm/lib/Linker/IRMover.cpp
1561

I'm not familiary enough with this code / LLVM to answer this. cc @gulfem who added this in D137360.

Anyway, marking this comment as "done" because if this needs to be fixed, it shouldn't be part of this change.

1577–1579

@tejohnson do you have any input on this?

(I last looked at this code in-depth in early December, but don't remember seeing anything relevant. ninja check-all is also clean and in a brief test IRMover would happily import a function with a different signature without creating any bitcasts).

TimNN marked an inline comment as done.Jan 13 2023, 7:00 AM
arsenm added inline comments.Jan 13 2023, 7:02 AM
llvm/lib/Linker/IRMover.cpp
1577–1579

These days it would appear as a callsite with a different type from the callee, there's no explicit bitcast with opaque pointers

TimNN added inline comments.Jan 13 2023, 7:25 AM
llvm/lib/Linker/IRMover.cpp
1577–1579

Ack, then I guess the "brief test" wasn't really useful.

Anyway, if it is indeed expected that SrcF and DstF have different signatures here, I'd appreciate some guidance on how to best detect and handle that.

tejohnson added inline comments.Jan 13 2023, 4:13 PM
llvm/lib/Linker/IRMover.cpp
1554–1556

How about just do an early return if !DstF, since your changes below also rely on DstF being non-null.

1561

Thanks for noting that, it is a limitation with the earlier patch that I missed. I will copy your comment over there.

1568

I think this is backwards? My understanding (also from the code and the test case) is that the issue occurs if F is a declaration in the dest module but was a definition in the source module.

1577–1579

Sorry I'm not really sure. @arsenm do you mean that the arg_size could be different? Do you have an example of how/when that would happen?

TimNN updated this revision to Diff 489239.Jan 14 2023, 6:43 AM
TimNN marked 2 inline comments as done.

Fix definition/declaration wording in comment.

llvm/lib/Linker/IRMover.cpp
1554–1556

Given the generic name ("updateAttributes") and purpose of this function, I decided to avoid any early returns.

(Put differently: This function currently has two mostly independent (and long) sections, each with their own prerequisites. I wanted to keep the control-flow contained to the individual sections, especially in light of the possibility that this function may handle non-function globals in the future).

I'd be happy to add the early return, but in that case I'd advocate for renaming the function "updateFunctionAttributes".

1568

Yes, of course. If I don't pay very close attention it's too easy to mix up the two. I've updated the patch.

tejohnson accepted this revision.Jan 18 2023, 7:47 AM

lgtm, but there is still an outstanding question to @arsenm. I'm not exactly sure of the scenario they are concerned about.

This revision is now accepted and ready to land.Jan 18 2023, 7:47 AM
TimNN marked 3 inline comments as done.Feb 1 2023, 10:27 AM
TimNN added inline comments.
llvm/lib/Linker/IRMover.cpp
1577–1579

Friendly ping, @arsenm! Could you provide more details about your concern, or even better, a concrete example?

aeubanks added inline comments.Feb 2 2023, 10:30 AM
llvm/lib/Linker/IRMover.cpp
1577–1579

I think what @arsenm is talking about is a separate thing that isn't relevant here. The callsite thing is on function calls (CallBase::getCalledFunction() returns nullptr if the call args and callee are mismatched), but this is about handling function declarations/definitions of the same function.

If there's any updating of callers of a function, that would be happening somewhere else, not in the code touched by this patch.

TimNN marked 2 inline comments as done.Feb 7 2023, 12:52 AM
TimNN added inline comments.
llvm/lib/Linker/IRMover.cpp
1577–1579

Than you for your input, @aeubanks!

@tejohnson: Would you be fine with submitting this patch as-is?

TimNN marked an inline comment as done.Feb 7 2023, 12:52 AM
tejohnson added inline comments.Feb 16 2023, 6:28 AM
llvm/lib/Linker/IRMover.cpp
1577–1579

Yes, lgtm

TimNN updated this revision to Diff 498410.Feb 17 2023, 9:25 AM

Rebase onto HEAD.

This revision was landed with ongoing or failed builds.Feb 21 2023, 9:43 AM
This revision was automatically updated to reflect the committed changes.

I believe we're seeing a stage 2 ThinLTO build of clang crash due to this patch, will reduce test case

aeubanks reopened this revision.Feb 22 2023, 9:40 AM

I believe we're seeing a stage 2 ThinLTO build of clang crash due to this patch, will reduce test case

confirmed locally, should be reproducible everywhere (saw the same crash on a windows bot)

reverting

This revision is now accepted and ready to land.Feb 22 2023, 9:40 AM

The current code is failing for a situation like this:

--- SRC ---
; Module Name: lib/libLLVMCodeGen.a(TargetLoweringBase.cpp.o at 275472348)
; Module Source: /home/logic/Projects/llvm/llvm/lib/CodeGen/TargetLoweringBase.cpp
; Materializable
; Function Attrs: mustprogress noinline nounwind optnone uwtable
define { ptr, i8 } @_ZNK4llvm18TargetLoweringBase23findRepresentativeClassEPKNS_18TargetRegisterInfoENS_3MVTE(ptr noundef nonnull align 8 dereferenceable(218195) %0, ptr noundef %1, i8 %2) unnamed_addr #1 align 2 {}

--- DST ---
; Module Name: lib/libLLVMAArch64CodeGen.a(AArch64ISelLowering.cpp.o at 48495784)
; Module Source: /home/logic/Projects/llvm/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
declare void @_ZNK4llvm18TargetLoweringBase23findRepresentativeClassEPKNS_18TargetRegisterInfoENS_3MVTE() unnamed_addr

It seems like when generating vtables, the code at https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGVTables.cpp may generate function declarations with an "empty/void" signature for the IsUnprototyped case.

Since clang is deliberately generating this code, I'm assuming this is considered valid according to LLVM IR semantics. I'm not sure what the most appropriate fix here would be. I primarily see three open questions:

Question 1: Is my assumption correct and this is valid LLVM IR?

Question 2: Should we attempt to detect this case specifically (DST has zero args and returns void) but otherwise still assert that the argument lists have the same length? Or should we handle all cases of argument lists with different lengths?

Question 3: For the cases detected by (1), should we conservatively remove UB-implying attributes from all arguments (erring on the side of correctness)? Or should we just bail out and not do anything (erring on the side of performance)?

The current code is failing for a situation like this:

--- SRC ---
; Module Name: lib/libLLVMCodeGen.a(TargetLoweringBase.cpp.o at 275472348)
; Module Source: /home/logic/Projects/llvm/llvm/lib/CodeGen/TargetLoweringBase.cpp
; Materializable
; Function Attrs: mustprogress noinline nounwind optnone uwtable
define { ptr, i8 } @_ZNK4llvm18TargetLoweringBase23findRepresentativeClassEPKNS_18TargetRegisterInfoENS_3MVTE(ptr noundef nonnull align 8 dereferenceable(218195) %0, ptr noundef %1, i8 %2) unnamed_addr #1 align 2 {}

--- DST ---
; Module Name: lib/libLLVMAArch64CodeGen.a(AArch64ISelLowering.cpp.o at 48495784)
; Module Source: /home/logic/Projects/llvm/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
declare void @_ZNK4llvm18TargetLoweringBase23findRepresentativeClassEPKNS_18TargetRegisterInfoENS_3MVTE() unnamed_addr

It seems like when generating vtables, the code at https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGVTables.cpp may generate function declarations with an "empty/void" signature for the IsUnprototyped case.

Since clang is deliberately generating this code, I'm assuming this is considered valid according to LLVM IR semantics. I'm not sure what the most appropriate fix here would be. I primarily see three open questions:

Question 1: Is my assumption correct and this is valid LLVM IR?

Yes I think we should assume so. I believe there may be other cases where the compiler may generate a function declaration without knowing the exact definition's arguments...I know WPD does this sometimes although it would be after importing so wouldn't trigger this particular code.

Question 2: Should we attempt to detect this case specifically (DST has zero args and returns void) but otherwise still assert that the argument lists have the same length? Or should we handle all cases of argument lists with different lengths?

I think it might be best to handle these cases conservatively, eg:

Question 3: For the cases detected by (1), should we conservatively remove UB-implying attributes from all arguments (erring on the side of correctness)? Or should we just bail out and not do anything (erring on the side of performance)?

I think the best option is likely to remove all UB implying attributes from the dst in that case. Note that in the example shown above, it should not need to do anything, since the dst is a declaration without any args, which I would guess would be the likely case when we are going to hit this situation.