[PATCH] D108123: [MemoryBuiltins] Mark user defined delete as nobuiltin

[guopeilin via Phabricator via llvm-commits]

guopeilin added a comment.

In D108123#2951595 <https://reviews.llvm.org/D108123#2951595>, @lebedev.ri wrote:

> 



> What if we are statically linking to the libc++ with LTO?
> Surely we'll have said definition available, even though it is not `nobuiltin`?

Thanks for reviewing. 
When I use the following command `clang++ -O0 test.cpp  -flto=full -fuse-ld=lld -Wl,-plugin-opt=save-temps -static -stdlib=libstdc++`, I can still only find the deletion function only have declaration without a definition in the file `*.preopt.bc` and file `*.internalize.bc`.
The test.cpp looks like this:

  int a = 0;
  int test() {
    int *p = new int(2);
    a = *p;
    delete p;
    return a;
  }
  
  int main(){
    a = test();
    return 1;
  }

The IR of looks like

  define dso_local i32 @_Z4testv() #0 {
  entry:
    %p = alloca i32*, align 8
    %call = call noalias nonnull i8* @_Znwm(i64 4) #4
    %0 = bitcast i8* %call to i32*
    store i32 2, i32* %0, align 4
    store i32* %0, i32** %p, align 8
    %1 = load i32*, i32** %p, align 8
    %2 = load i32, i32* %1, align 4
    store i32 %2, i32* @a, align 4
    %3 = load i32*, i32** %p, align 8
    %isnull = icmp eq i32* %3, null
    br i1 %isnull, label %delete.end, label %delete.notnull
  
  delete.notnull:                                   ; preds = %entry
    %4 = bitcast i32* %3 to i8*
    call void @_ZdlPv(i8* %4) #5
    br label %delete.end
  
  delete.end:                                       ; preds = %delete.notnull, %entry
    %5 = load i32, i32* @a, align 4
    ret i32 %5
  }
  
  ; Function Attrs: nobuiltin allocsize(0)
  declare dso_local nonnull i8* @_Znwm(i64) #1
  
  ; Function Attrs: nobuiltin nounwind
  declare dso_local void @_ZdlPv(i8*) #2
  
  ; Function Attrs: mustprogress noinline norecurse optnone uwtable
  define dso_local i32 @main() #3 {
  entry:
    %retval = alloca i32, align 4
    store i32 0, i32* %retval, align 4
    %call = call i32 @_Z4testv()
    store i32 %call, i32* @a, align 4
    ret i32 1
  }

The attribute of `#5` is `attributes #5 = { builtin nounwind }`. https://godbolt.org/z/14dPM9Yf7
So this patch may not have harm on the `statically linking` case cause we only take care of the function with an exact definition.
Please correct me if I get wrong.

> What if it is overloaded in another TU, or via `LD_PRELOAD`? We then won't think it's `nobuiltin`?

Yes, in such cases, we can not get the definition of the overloaded function.  This patch cannot cover this case. 
However, if we cannot find the definition of the deletion function, the `DeadArgumentElimination` won't change the argument into undef, thus we won't generate `free(undef)`  instruction,  which makes things become fine.

> I think this is a clang problem.

Yes, I agree with the point that clang has some bug. As far as I have investigated the IR that generated just after clang,  no matter whether we overload the delete function, clang will always add a `builtin` attribute to the instruction that calls the delete function. 
It seems that clang only takes the function name into consideration to decide whether a function is builtin or not.
And it would be a great appreciation if you can give any suggestion on clang debug.


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

https://reviews.llvm.org/D108123