[Bug lto/105727] New: __builtin_constant_p expansion in LTO

[Bug lto/105727] New: __builtin_constant_p expansion in LTO

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727

            Bug ID: 105727
           Summary: __builtin_constant_p expansion in LTO
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: lto
          Assignee: unassigned at gcc dot gnu.org
          Reporter: marxin at gcc dot gnu.org
                CC: hubicka at gcc dot gnu.org, marxin at gcc dot gnu.org
  Target Milestone: ---

It's basically follow-up of PR101941 which I noticed during the building of
Linux with LTO w/ KASAN enabled:

$ cat a.c
void memory_is_poisoned(int size) {
  if (__builtin_constant_p(size)) __builtin_abort();
}

$ cat b.c
void memory_is_poisoned(int size);

int main(int argc, char **argv) {
  memory_is_poisoned(128);
  return 0;
}

$ gcc [ab].c -O2 && ./a.out
$ clang [ab].c -flto -O2 && ./a.out
$ gcc [ab].c -flto -O2 && ./a.out
Aborted (core dumped)

[Bug lto/105727] __builtin_constant_p expansion in LTO

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727

Martin Liška <marxin at gcc dot gnu.org> 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 <marxin at gcc dot gnu.org> ---
So the question is if the LTO time expansion is correct or not?

[Bug lto/105727] __builtin_constant_p expansion in LTO

[fw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727

Florian Weimer <fw at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fw at gcc dot gnu.org

--- Comment #2 from Florian Weimer <fw at gcc dot gnu.org> ---
(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

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |WAITING

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
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

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
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

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|WAITING                     |RESOLVED

--- Comment #5 from Martin Liška <marxin at gcc dot gnu.org> ---
All right, then closing as invalid.

[Bug lto/105727] __builtin_constant_p expansion in LTO

[hubicka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727

--- Comment #6 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
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

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727

--- Comment #7 from Martin Liška <marxin at gcc dot gnu.org> ---
(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

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
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

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727

--- Comment #9 from Martin Liška <marxin at gcc dot gnu.org> ---
> 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

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
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

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727

--- Comment #11 from Martin Liška <marxin at gcc dot gnu.org> ---
(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.

Re: [Bug lto/105727] __builtin_constant_p expansion in LTO

[Jan Hubicka]

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

[hubicka at kam dot mff.cuni.cz]

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

[Bug lto/105727] __builtin_constant_p expansion in LTO

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727

--- Comment #13 from Martin Liška <marxin at gcc dot gnu.org> ---
> 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

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727

--- Comment #14 from Martin Liška <marxin at gcc dot gnu.org> ---
(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

[hubicka at kam dot mff.cuni.cz]

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

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105727

--- Comment #16 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
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.