A Call instruction with delete/free may be mark as builtin after clang,
however, in some cases user may overload these functions thus we should
mark them as nobuiltin.
Details
Details
Diff Detail
Diff Detail
Unit Tests
Unit Tests
| Time | Test | |
|---|---|---|
| 700 ms | x64 windows > Clang.Index::complete-preprocessor.m |
Event Timeline
| llvm/test/Transforms/InstCombine/free-undef.ll | ||
|---|---|---|
| 3 ↗ | (On Diff #366594) | I don't think runline adds value |
Comment Actions
Right, but that is the correct outcome.
Why is it not UB to free an undef (a.k.a. any random pointer)?
It is as per the current langref: https://alive2.llvm.org/ce/z/jBqe-E
Also, i would like to note that the patch's description doesn't mention that this wants to make free(undef) non-UB.
Comment Actions
The key point is that function free can be overload, take this test case as an example, we can overload function _ZdlPv with an empty body. So the result becomes that we call a free function which actually does not free anything. Absolutely this is not a way that free an undef.
So it is better to delete an instruction that calling an empty function with an undefined argument.
I will update this test case later, also the description.
Comment Actions
Thanks, but this still does not sound good to me.
If you want to change semantics of free()/_ZdlPv() in such a way,
what you need to do is to mark it as nobuiltin: https://godbolt.org/z/a5P3TzG7T
Comment Actions
Hi, I update a new patch that fix the attribute nobuiltin
The source code is like following:
int a = 0;
void operator delete(void *p) __attribute__((noinline));
void operator delete(void *p)
{
a = 1;
}
int test() {
int *p = new int(2);
a = *p;
delete p;
return a;
}
int main(){
a = test();
return 1;
}If we find a delete/free function with extact definition, then we can not treat them as builtin function any more.
Comment Actions
I think this is a clang problem.
What if we are statically linking to the libc++ with LTO?
Surely we'll have said definition avaliable, even though it is not nobuiltin?
What if it is overloaded in another TU, or via LD_PRELOAD? We then won't think it's nobuiltin?
Comment Actions
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.
Comment Actions
I was really talking about statically linking to libc++ which is also statically linked, and compiling everything with LTO,
then you should have the definition available. I don't know if that is a supported configuration.
If this is a clang bug, then presumably it should be fixed in clang, and not workarounded in middle-end,
penalizing everyone else who doesn't wish to change semantics in such a way,
Wasn't there some command-line argument to specify that a certain builtin isn't a builtin?
Comment Actions
Wasn't there some command-line argument to specify that a certain builtin isn't a builtin?
Yes, the option is -fno-builtin. This turns off all instruction combining on calls to recognized library calls. Adding this option does solve the problem, by changing the attributes on the tail call void @_ZdlPv(i8* undef) #6 instruction to attributes #6 = { builtin nobuiltin nounwind "no-builtins" }.
Clang also happily accepts -fno-builtin-delete, but it is ineffective; the IR produced by the frontend is the same as if it ran without -fno-builtin-delete. If -fno-builtin-delete (and -fno-builtin-new, etc.) is supported properly, it could be the correct solution for this problem. The programmer needs to tell Clang that delete is overloaded in some translation unit, and this option is the way to do so.
Comment Actions
See also https://bugs.llvm.org/show_bug.cgi?id=41039 .
The edge cases involving target library functions are rarely relevant... most of the functions are part of the C library, and that's usually built with -fno-builtin/-ffreestanding. But this is one of the cases where someone can define a "library" function in their code.
Basically, optimizations have to consistently make a decision here: is the function a builtin function, or is it something we can perform interprocedural optimizations on? Once we start doing interprocedural optimizations, we're potentially breaking the original semantics of the function, and therefore any optimization that treats the function as a builtin will get weird results.
Just saying "any function with a definition isn't a target library function" probably produces a sane result in most cases. If we're going to do that, though, we should do it in TargetLibraryInfo (in TargetLibraryInfo::getLibFunc?), not in llvm::isFreeCall.
(-fno-builtin-delete tells the compiler that a C library function named "delete" shouldn't be treated as builtin. But clang doesn't know of any such function, so -fno-builtin-delete doesn't do anything. The delete operator is not named "delete".)
Comment Actions
Just saying "any function with a definition isn't a target library function" probably produces a sane result in most cases.
That doesn't sound like the right solution though. If the programmer provides operators new and delete in some translation unit other than the one being compiled, without LTO, the definitions are not available in the current translation unit. I think @lebedev.ri was describing the same.
Moreover, __attribute__((no_builtin)) does not work on declarations, so the programmer currently has no way to tell Clang that these operators shouldn't be treated as builtins. Adding options such as -fno-builtin-operator-new and -fno-builtin-operator-delete sounds like the best solution to me.
As a reference point, GCC doesn't seem to optimize these operators at all.
Comment Actions
If the programmer provides operators new and delete in some translation unit other than the one being compiled, without LTO, the definitions are not available in the current translation unit.
I don't think there's a correctness issue in the case where the new and delete operators are defined in some other translation unit. The C++ standard says all the optimizations we want to do on new/delete expressions are legal whether or not the user replaces the operators . See [expr.new] (http://eel.is/c++draft/expr.new#12) . The weird issues this patch is trying to fix only show up when the definitions are available.
Adding options such as -fno-builtin-operator-new and -fno-builtin-operator-delete sounds like the best solution to me.
A flag that disables the optimizations allowed by [expr.new] might be nice to have, I guess. But it wouldn't really fix anything.
Comment Actions
The C++ standard says all the optimizations we want to do on new/delete expressions are legal whether or not the user replaces the operators . See [expr.new] (http://eel.is/c++draft/expr.new#12) . The weird issues this patch is trying to fix only show up when the definitions are available.
Thanks for linking the standard; I wasn't aware of the legality to optimize calls to such replaced functions. According to that wording, wouldn't we be permitted to omit calls to the overloaded new/delete operators even when the definitions are available? If we take that stance, there is no problem, is there? Perhaps we should emit an warning that such operator definitions can and will be ignored.
Comment Actions
The recent changes on handling inline builtins doesn't affect this patch. To be considered as an inline builtin, a function must be marked as inline always_inline gnu_inline
clang-format: please reformat the code