[Bug middle-end/77964] [7 Regression] Linux kernel firmware loader miscompiled

[pinskia at gcc dot gnu.org via Gcc-bugs]

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

Andrew Pinski  changed:

   What|Removed |Added

 CC||neoxic at icloud dot com

--- Comment #19 from Andrew Pinski  ---
*** Bug 106939 has been marked as a duplicate of this bug. ***

[Bug middle-end/77964] [7 Regression] Linux kernel firmware loader miscompiled

[rguenth at gcc dot gnu.org]

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

--- Comment #18 from Richard Biener  ---
Author: rguenth
Date: Wed Nov  2 08:29:48 2016
New Revision: 241776

URL: https://gcc.gnu.org/viewcvs?rev=241776=gcc=rev
Log:
2016-11-02  Richard Biener  

PR tree-optimization/78035
PR tree-optimization/77964
* gimple-pretty-print.c (pp_points_to_solution): Print
vars_contains_interposable.
* tree-ssa-alias.c: Include varasm.h.
(ptrs_compare_unequal): Check vars_contains_interposable and
decl_binds_to_current_def_p.
(dump_points_to_solution): Dump vars_contains_interposable.
* tree-ssa-alias.h (struct pt_solution): Add vars_contains_interposable
flag.
* tree-ssa-structalias.c: Include varasm.h.
(set_uids_in_ptset): Record whether vars contains a
not decl_binds_to_current_def_p variable in vars_contains_interposable.
(ipa_escaped_pt): Update initializer.

* gcc.target/i386/pr78035.c: New testcase.

Added:
trunk/gcc/testsuite/gcc.target/i386/pr78035.c
Modified:
trunk/gcc/ChangeLog
trunk/gcc/gimple-pretty-print.c
trunk/gcc/testsuite/ChangeLog
trunk/gcc/tree-ssa-alias.c
trunk/gcc/tree-ssa-alias.h
trunk/gcc/tree-ssa-structalias.c

[Bug middle-end/77964] [7 Regression] Linux kernel firmware loader miscompiled

[jakub at gcc dot gnu.org]

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

--- Comment #17 from Jakub Jelinek  ---
(In reply to Jiri Slaby from comment #16)
> (In reply to Jakub Jelinek from comment #15)
> > lots of them that rely on pointer arithmetics being defined only within the
> > same object.
> 
> Sure, but the two pointers (taken implicitly of the arrays) are within the
> same object. So I do not see, why it wouldn't work? I.e. where exactly this
> breaks the C specs?

No.  In C
extern struct builtin_fw __start_builtin_fw[];
extern struct builtin_fw __end_builtin_fw[];
declares two external arrays, thus they are two independent objects.  It is
like if you have:
int a[10];
int b[10];
in your program, although they might be allocated adjacent, such that
int *p = [10]; int *q = [0]; memcmp (, , sizeof (p)) == 0;
[0] - [0] is still UB.
What you do with __start_*/__end_* symbols is nothing you can define in C, you
need linker support or asm for that, and to use it without UB you also need to
use an optimization barrier that has been suggested.

[Bug middle-end/77964] [7 Regression] Linux kernel firmware loader miscompiled

[jirislaby at gmail dot com]

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

--- Comment #16 from Jiri Slaby  ---
(In reply to Jakub Jelinek from comment #15)
> lots of them that rely on pointer arithmetics being defined only within the
> same object.

Sure, but the two pointers (taken implicitly of the arrays) are within the
same object. So I do not see, why it wouldn't work? I.e. where exactly this
breaks the C specs?

[Bug middle-end/77964] [7 Regression] Linux kernel firmware loader miscompiled

[jakub at gcc dot gnu.org]

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

--- Comment #15 from Jakub Jelinek  ---
(In reply to Jiri Slaby from comment #14)
> (In reply to Andrew Pinski from comment #10)
> > (In reply to Markus Trippelsdorf from comment #9)
> > > Is subtracting undefined, too?
> > Yes.  Comparing two unrelated arrays or subtracting them is undefined.
> 
> But they are not unrelated arrays. So what from the C standard actually
> makes (and allows) gcc think they are unrelated?

C doesn't have any notion of "related" declarations.

> And given gcc 7 is to be released yet, can we have a switch to disable this
> optimization?

This is nothing new in GCC 7, you've most likely just been extremely lucky in
the past that it happened to work as you expected.  Other projects had to
change similar UB code years ago.  It isn't just a single optimization, but
lots of them that rely on pointer arithmetics being defined only within the
same object.

[Bug middle-end/77964] [7 Regression] Linux kernel firmware loader miscompiled

[jirislaby at gmail dot com]

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

--- Comment #14 from Jiri Slaby  ---
(In reply to Andrew Pinski from comment #10)
> (In reply to Markus Trippelsdorf from comment #9)
> > Is subtracting undefined, too?
> Yes.  Comparing two unrelated arrays or subtracting them is undefined.

But they are not unrelated arrays. So what from the C standard actually makes
(and allows) gcc think they are unrelated?

And given gcc 7 is to be released yet, can we have a switch to disable this
optimization?

[Bug middle-end/77964] [7 Regression] Linux kernel firmware loader miscompiled

[trippels at gcc dot gnu.org]

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

Markus Trippelsdorf  changed:

   What|Removed |Added

URL||http://marc.info/?t=1466867
   ||2242=1=2

--- Comment #13 from Markus Trippelsdorf  ---
See: http://marc.info/?t=14668672242=1=2 for kernel side discussion.

[Bug middle-end/77964] [7 Regression] Linux kernel firmware loader miscompiled

[pinskia at gcc dot gnu.org]

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

Andrew Pinski  changed:

   What|Removed |Added

 CC||jirislaby at gmail dot com

--- Comment #12 from Andrew Pinski  ---
*** Bug 77981 has been marked as a duplicate of this bug. ***

[Bug middle-end/77964] [7 Regression] Linux kernel firmware loader miscompiled

[glisse at gcc dot gnu.org]

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

--- Comment #11 from Marc Glisse  ---
(In reply to Andrew Pinski from comment #5)
> Looks like a kernel issue. An asm ("", "+r"(x)); is needed in the source for
> __start_builtin_fw and __end_builtin_fw

Shouldn't we recommend "+g" instead of "+r"?

(In reply to Markus Trippelsdorf from comment #9)
> Is subtracting undefined, too?
> 
> -   for (b_fw = __start_builtin_fw; b_fw != __end_builtin_fw; b_fw++) {
> +   for (b_fw = __start_builtin_fw;  __end_builtin_fw - b_fw; b_fw++) {

See bug 77813.

[Bug middle-end/77964] [7 Regression] Linux kernel firmware loader miscompiled

[pinskia at gcc dot gnu.org]

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

--- Comment #10 from Andrew Pinski  ---
(In reply to Markus Trippelsdorf from comment #9)
> Is subtracting undefined, too?
Yes.  Comparing two unrelated arrays or subtracting them is undefined.

[Bug middle-end/77964] [7 Regression] Linux kernel firmware loader miscompiled

[trippels at gcc dot gnu.org]

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

Markus Trippelsdorf  changed:

   What|Removed |Added

 Status|WAITING |RESOLVED
 Resolution|--- |INVALID

--- Comment #9 from Markus Trippelsdorf  ---
Is subtracting undefined, too?

-   for (b_fw = __start_builtin_fw; b_fw != __end_builtin_fw; b_fw++) {
+   for (b_fw = __start_builtin_fw;  __end_builtin_fw - b_fw; b_fw++) {

[Bug middle-end/77964] [7 Regression] Linux kernel firmware loader miscompiled

[pinskia at gcc dot gnu.org]

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

--- Comment #8 from Andrew Pinski  ---
(In reply to Jakub Jelinek from comment #7)
> You don't need it for both.
>   struct builtin_fw *b_fw = __start_builtin_fw;
>   asm ("" : "+r" (b_fw));
>   for (; b_fw != __end_builtin_fw; b_fw++) {
> should be enough.  And indeed, without that it is undefined behavior.

You might want to do end only then instead of the start.  Because in theory GCC
could say it is always false as &__end_builtin_fw[-1] is undefined behavior too
so GCC say if the initial value was not __end_builtin_fw[0] go into an infinite
loop.

[Bug middle-end/77964] [7 Regression] Linux kernel firmware loader miscompiled

[jakub at gcc dot gnu.org]

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

--- Comment #7 from Jakub Jelinek  ---
You don't need it for both.
  struct builtin_fw *b_fw = __start_builtin_fw;
  asm ("" : "+r" (b_fw));
  for (; b_fw != __end_builtin_fw; b_fw++) {
should be enough.  And indeed, without that it is undefined behavior.

[Bug middle-end/77964] [7 Regression] Linux kernel firmware loader miscompiled

[pinskia at gcc dot gnu.org]

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

--- Comment #6 from Andrew Pinski  ---
That is change:
extern struct builtin_fw __start_builtin_fw[];
extern struct builtin_fw __end_builtin_fw[];

static bool fw_get_builtin_firmware(struct firmware *fw, const char *name,
void *buf, size_t size)
{
 struct builtin_fw *b_fw;

 for (b_fw = __start_builtin_fw; b_fw != __end_builtin_fw; b_fw++) {
--- CUT --

to (there might already be a macro in linux which does the asm like below
already):

extern struct builtin_fw __start_builtin_fw[];
extern struct builtin_fw __end_builtin_fw[];

static bool fw_get_builtin_firmware(struct firmware *fw, const char *name,
void *buf, size_t size)
{
 struct builtin_fw *b_fw;
 struct builtin_fw *b_fw_start = __start_builtin_fw, b_fw_end =
__end_builtin_fw;
 asm("":"+r"(b_fw_start));
 asm("":"+r"(b_fw_end));


 for (b_fw = b_fw_start; b_fw != b_fw_end; b_fw++) {

[Bug middle-end/77964] [7 Regression] Linux kernel firmware loader miscompiled

[pinskia at gcc dot gnu.org]

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

Andrew Pinski  changed:

   What|Removed |Added

 Status|NEW |WAITING

--- Comment #5 from Andrew Pinski  ---
Looks like a kernel issue. An asm ("", "+r"(x)); is needed in the source for
__start_builtin_fw and __end_builtin_fw

[Bug middle-end/77964] [7 Regression] Linux kernel firmware loader miscompiled

[trippels at gcc dot gnu.org]

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

Markus Trippelsdorf  changed:

   What|Removed |Added

 Status|UNCONFIRMED |NEW
   Last reconfirmed||2016-10-13
 Ever confirmed|0   |1

--- Comment #4 from Markus Trippelsdorf  ---
Started with r235622:

commit 73447cc5d17178b0a756be48133e55fdc7574c13
Author: rguenth 
Date:   Fri Apr 29 08:36:49 2016 +

2016-04-29  Richard Biener  

PR tree-optimization/13962
PR tree-optimization/65686
* tree-ssa-alias.h (ptrs_compare_unequal): Declare.
* tree-ssa-alias.c (ptrs_compare_unequal): New function
using PTA to compare pointers.
* match.pd: Add pattern for pointer equality compare simplification
using ptrs_compare_unequal

good:
callkmem_cache_alloc
testq   %rax, %rax  
movq%rax, %r12  
je  .L28
movq$__start_builtin_fw, %r13   
cmpq$__end_builtin_fw, %r13 
jne .L86
jmp .L30
.L32:   
addq$24, %r13   
cmpq$__end_builtin_fw, %r13 
je  .L30
.L86:   
movq0(%r13), %rsi   
movq%rbx, %rdi  
callstrcmp  

--

bad:
callkmem_cache_alloc
testq   %rax, %rax  
movq%rax, %r15  
movq(%rsp), %r8 
je  .L6 
movq$__start_builtin_fw, %rbx   
jmp .L7 
.L20:   
addq$24, %rbx   
.L7:
movq(%rbx), %rsi
movq%rbp, %rdi  
callstrcmp

[Bug middle-end/77964] [7 Regression] Linux kernel firmware loader miscompiled

[trippels at gcc dot gnu.org]

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

--- Comment #3 from Markus Trippelsdorf  ---
(In reply to Jakub Jelinek from comment #2)
> Can you bisect? Suspect changes might be the shrink wrapping changes from
> last night - r241063 and r241059-r241061.

It is much older issue. Even gcc from 20160928 fails.

And bisecting is annoying, because unfortunately the failure only happens
on bare metal (booting under qemu is fine). So I have to reboot all the time.

I will try to bisect by looking at the assembly output.

[Bug middle-end/77964] [7 Regression] Linux kernel firmware loader miscompiled

[jakub at gcc dot gnu.org]

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

Jakub Jelinek  changed:

   What|Removed |Added

 CC||jakub at gcc dot gnu.org

--- Comment #2 from Jakub Jelinek  ---
Can you bisect? Suspect changes might be the shrink wrapping changes from last
night - r241063 and r241059-r241061.