[Bug lto/105727] __builtin_constant_p expansion in LTO
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727 --- Comment #16 from Jakub Jelinek --- That may be true, but I think only the 1/2/4/8/16 sizes are interesting to handle with special code. And as the function is provably called by a function which can have any size and through LTO can get a constant value, the options are remove the BUILD_BUG(); or ensure that the size is never constant from the mm/kasan/shadow.c (memset) call (hide e.g. through inline asm the exact value from the compiler) or have 2 different implementations, one that has BUILD_BUG() and is used only for those specific sizes and another one which doesn't (either calls just the *_n function or has everything but BUILD_BUG) that can handle arbitrary sizes.
[Bug lto/105727] __builtin_constant_p expansion in LTO
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727 --- Comment #15 from hubicka at kam dot mff.cuni.cz --- > No, see c#10. I know it will work if BUILD_BUG call is removed. However the only reason I can see why original author put it there is that he/she wanted to write special case checkers for constants that appears in practice and wanted unhandled constants to turn to compiler errors rather than quietly going the slow path...
[Bug lto/105727] __builtin_constant_p expansion in LTO
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727 --- Comment #14 from Martin Liška --- (In reply to Martin Liška from comment #13) > > To me it looked like a protection that size is not going to be large > > (or perhaps author wants to add extra special cases as they are needed) > > No, see c#10. comment 10
[Bug lto/105727] __builtin_constant_p expansion in LTO
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727 --- Comment #13 from Martin Liška --- > To me it looked like a protection that size is not going to be large > (or perhaps author wants to add extra special cases as they are needed) No, see c#10.
[Bug lto/105727] __builtin_constant_p expansion in LTO
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727 --- Comment #12 from hubicka at kam dot mff.cuni.cz --- > > My guess is that the > > BUILD_BUG(); > > line is the sole thing that is wrong, it should be just break; > > as the memory_is_poisoned_n(addr, size); will handle all the sizes, > > regardless if they are constants or not. > > Sure, I'm going to suggest such a change. To me it looked like a protection that size is not going to be large (or perhaps author wants to add extra special cases as they are needed) Honza
Re: [Bug lto/105727] __builtin_constant_p expansion in LTO
> > My guess is that the > > BUILD_BUG(); > > line is the sole thing that is wrong, it should be just break; > > as the memory_is_poisoned_n(addr, size); will handle all the sizes, > > regardless if they are constants or not. > > Sure, I'm going to suggest such a change. To me it looked like a protection that size is not going to be large (or perhaps author wants to add extra special cases as they are needed) Honza
[Bug lto/105727] __builtin_constant_p expansion in LTO
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727 --- Comment #11 from Martin Liška --- (In reply to Jakub Jelinek from comment #10) > My guess is that the > BUILD_BUG(); > line is the sole thing that is wrong, it should be just break; > as the memory_is_poisoned_n(addr, size); will handle all the sizes, > regardless if they are constants or not. Sure, I'm going to suggest such a change.
[Bug lto/105727] __builtin_constant_p expansion in LTO
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727 --- Comment #10 from Jakub Jelinek --- My guess is that the BUILD_BUG(); line is the sole thing that is wrong, it should be just break; as the memory_is_poisoned_n(addr, size); will handle all the sizes, regardless if they are constants or not.
[Bug lto/105727] __builtin_constant_p expansion in LTO
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727
--- Comment #9 from Martin Liška ---
> Sure, we might not inline or ipa cp/vrp all of them...
That's the explanation as
bool kasan_check_range(unsigned long addr, size_t size, bool write,
unsigned long ret_ip)
{
return check_region_inline(addr, size, write, ret_ip);
}
lives in mm/kasan/generic.c and so one needs LTO in other to propagate to this
call.
[Bug lto/105727] __builtin_constant_p expansion in LTO
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727 Jakub Jelinek changed: What|Removed |Added CC||jakub at gcc dot gnu.org --- Comment #8 from Jakub Jelinek --- I must be missing something, but it looks to me like a kasan kernel bug memset(info, 0, sizeof(struct sysinfo)); most likely doesn't have size 1, 2, 4, 8 or 16 but it is still a constant size memset calls if (!kasan_check_range((unsigned long)addr, len, true, _RET_IP_)) that calls return check_region_inline(addr, size, write, ret_ip); in check_region_inline I don't see any effort to take a different path for sizes other than 1/2/4/8/16, then it calls if (likely(!memory_is_poisoned(addr, size))) and that function emits BUILD_BUG if size is constant and not 1/2/4/8/16 What just surprises me is that it doesn't hit much more often, there will be many memset calls with constant size that is not 1/2/4/8/16. Sure, we might not inline or ipa cp/vrp all of them...
[Bug lto/105727] __builtin_constant_p expansion in LTO
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727
--- Comment #7 from Martin Liška ---
(In reply to Jan Hubicka from comment #6)
> I don't know what clang does, but GCC keeps builtin_constant_p till late
> optimization and resolves it then. So here we cross module inline (or
> constant propagate) and then it becomes constant.
>
> Outcome of __builtin_constant_p should depend on optimization settings and
> LTO is one of them. So I would say that both clang and gcc are correct, just
> gcc has better QOI. Why this breaks kernel?
Using KASAN, we end up with:
In function ‘memory_is_poisoned’,
inlined from ‘check_region_inline’ at mm/kasan/generic.c:180:6,
inlined from ‘kasan_check_range’ at mm/kasan/generic.c:189:9,
inlined from ‘memset’ at mm/kasan/shadow.c:44:7,
inlined from ‘do_sysinfo.isra’ at kernel/sys.c:2656:2:
mm/kasan/generic.c:155:25: error: call to ‘__compiletime_assert_243’ declared
with attribute error: BUILD_BUG failed
155 | BUILD_BUG();
static __always_inline bool memory_is_poisoned(unsigned long addr, size_t size)
{
if (__builtin_constant_p(size)) {
switch (size) {
case 1:
return memory_is_poisoned_1(addr);
case 2:
case 4:
case 8:
return memory_is_poisoned_2_4_8(addr, size);
case 16:
return memory_is_poisoned_16(addr);
default:
BUILD_BUG();
}
}
return memory_is_poisoned_n(addr, size);
}
where we propagate large some value during LTRANS and that's why we end up with
BUILD_BUG().
[Bug lto/105727] __builtin_constant_p expansion in LTO
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727 --- Comment #6 from Jan Hubicka --- I don't know what clang does, but GCC keeps builtin_constant_p till late optimization and resolves it then. So here we cross module inline (or constant propagate) and then it becomes constant. Outcome of __builtin_constant_p should depend on optimization settings and LTO is one of them. So I would say that both clang and gcc are correct, just gcc has better QOI. Why this breaks kernel?
[Bug lto/105727] __builtin_constant_p expansion in LTO
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727 Martin Liška changed: What|Removed |Added Resolution|--- |INVALID Status|WAITING |RESOLVED --- Comment #5 from Martin Liška --- All right, then closing as invalid.
[Bug lto/105727] __builtin_constant_p expansion in LTO
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727 --- Comment #4 from Jakub Jelinek --- I don't see anything wrong on it. If memory_is_poisoned is inlined into main, size is constant, if memory_is_poisoned is cloned and ipa-cp or ipa-vrp optimized, likewise.
[Bug lto/105727] __builtin_constant_p expansion in LTO
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727 Andrew Pinski changed: What|Removed |Added Status|NEW |WAITING --- Comment #3 from Andrew Pinski --- This has to have been reduced too much. Gcc is doing exactly what it was asked to do. __builtin_constant_p works across inlining and lto.
[Bug lto/105727] __builtin_constant_p expansion in LTO
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727 Florian Weimer changed: What|Removed |Added CC||fw at gcc dot gnu.org --- Comment #2 from Florian Weimer --- (In reply to Martin Liška from comment #1) > So the question is if the LTO time expansion is correct or not? What's the argument in favor of it being incorrect? Sorry, I just don't see it.
[Bug lto/105727] __builtin_constant_p expansion in LTO
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727 Martin Liška changed: What|Removed |Added Ever confirmed|0 |1 See Also||https://gcc.gnu.org/bugzill ||a/show_bug.cgi?id=101941 Status|UNCONFIRMED |NEW Last reconfirmed||2022-05-25 --- Comment #1 from Martin Liška --- So the question is if the LTO time expansion is correct or not?
