[PATCH] D139209: [IRMover] Remove UB implying parameter attributes when necessary

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D139209#4146484 <https://reviews.llvm.org/D139209#4146484>, @TimNN wrote:

> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139209