So the question is if the LTO time expansion is correct or not?

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

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.

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.

All right, then closing as invalid.

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?

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

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

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

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.

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

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

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

(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

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

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.