[PATCH] D103288: [SanCov] Properly set ABI parameter attributes

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added subscribers: dblaikie, jonpa.
jonpa added a comment.

I ran the verifier extension (D103412 <https://reviews.llvm.org/D103412>) on SystemZ/SPEC17, and got several failures. I reduced one as a test case:

  declare dso_local i8* @strchr(i8*, i32) local_unnamed_addr #1
  declare dso_local i8* @memchr(i8*, i32 signext, i64)
  declare void @llvm.assume(i1 noundef)
  
  @0 = private unnamed_addr constant [21 x i8] c"$&*(){}[]'\22;\\|?<>~`\0A\00", align 2
  
  define i1 @Perl_do_exec3(i8* %arg, i32 %arg1, i32 %arg2) {
  bb:
    %i = call i8* @strchr(i8* getelementptr inbounds ([21 x i8], [21 x i8]* @0, i64 0, i64 0), i32 signext undef) #4
    %i3 = icmp ne i8* %i, null
    call void @llvm.assume(i1 %i3)
    br label %bb5
  
  bb5:                                              ; preds = %bb
    ret i1 undef
  }



  bin/opt -march=z14 -S -o - -enable-new-pm=0 -instcombine

gives

  argument to signext parameter requires signext attribute
    %memchr = call i8* @memchr(i8* noundef nonnull dereferenceable(21) getelementptr inbounds ([21 x i8], [21 x i8]* @0, i64 0, i64 0), i32 undef, i64 21)
  in function Perl_do_exec3
  LLVM ERROR: Broken function found, compilation aborted!

I tried this patch, which helped me with the test case:

  --- a/llvm/lib/IR/Instructions.cpp
  +++ b/llvm/lib/IR/Instructions.cpp
  @@ -501,6 +501,9 @@ void CallInst::init(FunctionType *FTy, Value *Func, ArrayRef<Value *> Args,
     (void)It;
     assert(It + 1 == op_end() && "Should add up!");
   
  + if (Function *Fun = dyn_cast<Function>(Func))
  +   setAttributes(Fun->getAttributes());
  +
     setName(NameStr);
   }

I saw the question as to why not do this in IRBuilder with the response that "It would be unintuitive to only sometimes copy attributes depending on whether the call is direct or not.", but there didn't seem to be some consensus on this. I would be one to agree that it seems very desirable to have this handled generally and not just hope that everyone will always remember this. It seems to me it could be done in the CallInst/InvokeInst constructors even per above.  If somebody has the intent of *not* adding the attributes to the CallInst (don't understand why), there could be an option to not do so - like an argument 'bool CopyAttributes = true'.

I am new to this discussion so I may be missing something... I guess I don't fully understand why the declaration is no longer used for attributes - can the same function be called with/without e.g. a ZExt attribute on a parameter? I thought the callee expects this always per the ABI and that it would be an error to omit it..?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103288