Created attachment 59393 [details]
Simplified testcase

Very simple testcase

Comment on attachment 59393 [details]
Simplified testcase

void
f ()
{
  volatile char c[0];
  c[0] = 0;
}

Maybe this one is related to the fact that LRA doesn't set strict when it is in strict-RTL mode?  For example, with your latest test case:

$avr-gcc foo.c -S -mlra -O1 -mmcu=atmega128 -fdump-rtl-final -mlog=legitimate_address_p

...
avr_legitimate_address_p[f:reload(322)]: ret=1, mode=QI strict=0 reload_completed=0 ra_in_progress=1 (reg_renumber):(r28 ---> r28)
(plus:HI (reg/f:HI 28 r28)
    (const_int 1 [0x1]))

avr_legitimate_address_p[f:reload(322)]: ret=1, mode=QI strict=0 reload_completed=0 ra_in_progress=1 (reg_renumber):(r28 ---> r28)
(plus:HI (reg/f:HI 28 r28)
    (const_int 1 [0x1]))

avr_legitimate_address_p[f:reload(322)]: ret=1, mode=QI strict=0 reload_completed=1 ra_in_progress=0 (reg_renumber):(r32 ---> r32)
(plus:HI (reg/f:HI 32 __SP_L__)
    (const_int 1 [0x1]))

avr_legitimate_address_p[f:postreload(323)]: ret=1, mode=QI strict=0 reload_completed=1 ra_in_progress=0 (reg_renumber):(r32 ---> r32)
(plus:HI (reg/f:HI 32 __SP_L__)
    (const_int 1 [0x1]))

avr_legitimate_address_p[f:split2(327)]: ret=1, mode=QI strict=0 reload_completed=1 ra_in_progress=0 (reg_renumber):(r32 ---> r32)
(plus:HI (reg/f:HI 32 __SP_L__)
    (const_int 1 [0x1]))

...

i.e. strict=0 in all calls, and hence the backend returns ret=1 for SP+const addresses.

After IRA we have:
---------------------------------------
;; bb 2 artificial_defs: { }
;; bb 2 artificial_uses: { u-1(28){ }u-1(32){ }u-1(34){ }}
;; lr  in  28 [r28] 29 [r29] 32 [__SP_L__] 34 [argL]
;; lr  use 28 [r28] 29 [r29] 32 [__SP_L__] 34 [argL]
;; lr  def
(note 3 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK)
(note 2 3 5 2 NOTE_INSN_FUNCTION_BEG)
(insn 5 2 0 2 (set (mem/v:QI (plus:HI (reg/f:HI 28 r28)
                (const_int 1 [0x1])) [0 c[0]+0 S1 A8])
        (const_int 0 [0])) "a.c":5:8 86 {movqi_insn_split}
     (expr_list:REG_DEAD (reg:QI 29 r29)
        (nil)))
;;  succ:       EXIT [always]  count:1073741824 (estimated locally, freq 1.0000) (FALLTHRU) a.c:6:1
;; lr  out 28 [r28] 32 [__SP_L__] 34 [argL]
---------------------------------------

We have only one executable insn in test function `a' after IRA:
(insn 5 2 0 2 (set (mem/v:QI (plus:HI (reg/f:HI 28 r28)
                (const_int 1 [0x1])) [0 c[0]+0 S1 A8])
        (const_int 0 [0])) "a.c":5:8 86 {movqi_insn_split}
     (expr_list:REG_DEAD (reg:QI 29 r29)
        (nil)))

(reg:HI 28) is a frame pointer register Y or R28,r29 
LRA tries to eliminate frame pointer to stack pointer and uses TARGET_FRAME_POINTER_REQUIRED hook.

The hook:
static bool
avr_frame_pointer_required_p (void) 
{
  return (cfun->calls_alloca
  || cfun->calls_setjmp
  || cfun->has_nonlocal_label
  || crtl->args.info.has_stack_args
  || get_frame_size () > 0);
}

The hook return `true' because frame size is 0.

After LRA we have:
---------------------------------------
;; bb 2 artificial_defs: { }
;; bb 2 artificial_uses: { u-1(32){ }}
;; lr  in  32 [__SP_L__] 33 [__SP_H__]
;; lr  use 32 [__SP_L__] 33 [__SP_H__]
;; lr  def
(note 3 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK)
(note 2 3 5 2 NOTE_INSN_FUNCTION_BEG)
(insn 5 2 8 2 (set (mem/v:QI (plus:HI (reg/f:HI 32 __SP_L__)
                (const_int 1 [0x1])) [0 c[0]+0 S1 A8])
        (const_int 0 [0])) "a.c":5:8 86 {movqi_insn_split}
     (nil))
;;  succ:       EXIT [always]  count:1073741824 (estimated locally, freq 1.0000) (FALLTHRU) a.c:6:1
;; lr  out 32 [__SP_L__]
---------------------------------------

insn 5 has a wrong address. AVR can't use (reg/f:HI 32 __SP_L__) as an address register.

Probably we have two bugs:
1. AVR port assumes that LRA (or reload) can eliminate frame pointer to stack pointer (if no args and frame size = 0). It's wrong for this testcase;
2. LRA made a wrong elimination (may be because of AVR port have a wrong insn definition or something else)

(In reply to Georg-Johann Lay from comment #3)
> Maybe this one is related to the fact that LRA doesn't set strict when it is
> in strict-RTL mode?  For example, with your latest test case:
> 
> $avr-gcc foo.c -S -mlra -O1 -mmcu=atmega128 -fdump-rtl-final
> -mlog=legitimate_address_p
> 
> ...
> avr_legitimate_address_p[f:reload(322)]: ret=1, mode=QI strict=0
> reload_completed=0 ra_in_progress=1 (reg_renumber):(r28 ---> r28)
> (plus:HI (reg/f:HI 28 r28)
>     (const_int 1 [0x1]))
> 
> avr_legitimate_address_p[f:reload(322)]: ret=1, mode=QI strict=0
> reload_completed=0 ra_in_progress=1 (reg_renumber):(r28 ---> r28)
> (plus:HI (reg/f:HI 28 r28)
>     (const_int 1 [0x1]))
> 
> avr_legitimate_address_p[f:reload(322)]: ret=1, mode=QI strict=0
> reload_completed=1 ra_in_progress=0 (reg_renumber):(r32 ---> r32)
> (plus:HI (reg/f:HI 32 __SP_L__)
>     (const_int 1 [0x1]))
> 
> avr_legitimate_address_p[f:postreload(323)]: ret=1, mode=QI strict=0
> reload_completed=1 ra_in_progress=0 (reg_renumber):(r32 ---> r32)
> (plus:HI (reg/f:HI 32 __SP_L__)
>     (const_int 1 [0x1]))
> 
> avr_legitimate_address_p[f:split2(327)]: ret=1, mode=QI strict=0
> reload_completed=1 ra_in_progress=0 (reg_renumber):(r32 ---> r32)
> (plus:HI (reg/f:HI 32 __SP_L__)
>     (const_int 1 [0x1]))
> 
> ...
> 
> i.e. strict=0 in all calls, and hence the backend returns ret=1 for SP+const
> addresses.

Our messages crossed.
I have to analyze your information.

At least, our main problem is that we have a frame pointer usage without frame size.

Clarification for simplified test case:
1. frame size is 0 (because a declaration of a local array has a zero size);
2. we have a local variable which can be addressable (althought it have a zero size)

(In reply to denisc from comment #2)
> Comment on attachment 59393 [details]
> Simplified testcase
> 
> void
> f ()
> {
>   volatile char c[0];
>   c[0] = 0;
> }

That is invalid C code, of course (an out of bounds access).

(In reply to Segher Boessenkool from comment #8)
> That is invalid C code, of course (an out of bounds access).
What about the other test case https://gcc.gnu.org/bugzilla/attachment.cgi?id=59148 ?

That's from an already existing test case for PR64088, so why has an ICE been fixed for that PR instead of closing it as invalid? -- Or ar least rejecting that test case?

...hmmm https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html reads: <quote>

6.18 Arrays of Length Zero

Declaring zero-length arrays is allowed in GNU C as an extension. A zero-length array can be useful as the last element of a structure that is really a header for a variable-length object:

[...]

Declaring zero-length arrays in other contexts, including [...] as non-member objects, is discouraged. Accessing elements of zero-length arrays declared in such contexts is undefined and may be diagnosed. 

</quote>

So the test case for PR64088 is already invalid?

(In reply to Segher Boessenkool from comment #8)
> (In reply to denisc from comment #2)
> > Comment on attachment 59393 [details]
> > Simplified testcase
> > 
> > void
> > f ()
> > {
> >   volatile char c[0];
> >   c[0] = 0;
> > }
> 
> That is invalid C code, of course (an out of bounds access).

I don't think that it's invalid.(maybe I'm wrong)

Segher, can you suggest how to obtain information (from internal GCC structures) about such cases ?
ie rewrite TARGET_FRAME_POINTER_REQUIRED hook so that it takes into account that we have an access to frame but frame-size is zero ?

The current hook:
static bool
avr_frame_pointer_required_p (void) 
{
  return (cfun->calls_alloca
  || cfun->calls_setjmp
  || cfun->has_nonlocal_label
  || crtl->args.info.has_stack_args
  || get_frame_size () > 0);
}

(In reply to denisc from comment #11)
> (In reply to Segher Boessenkool from comment #8)
> > (In reply to denisc from comment #2)
> > > Comment on attachment 59393 [details]
> > > Simplified testcase
> > > 
> > > void
> > > f ()
> > > {
> > >   volatile char c[0];
> > >   c[0] = 0;
> > > }
> > 
> > That is invalid C code, of course (an out of bounds access).
> 
> I don't think that it's invalid.(maybe I'm wrong)

You access elt 0 of the array, but the array has *no* elements.

(In reply to Georg-Johann Lay from comment #9)
> (In reply to Segher Boessenkool from comment #8)
> > That is invalid C code, of course (an out of bounds access).
> What about the other test case
> https://gcc.gnu.org/bugzilla/attachment.cgi?id=59148 ?

Same problem there.  It is not valid C.

> That's from an already existing test case for PR64088, so why has an ICE
> been fixed for that PR instead of closing it as invalid? -- Or ar least
> rejecting that test case?

Many such problems cannot ever be diagnosed by the compiler.  Simple cases like
here could be of course, but in general it is the halting problem.

It is fine to have testcases that are invalid C, if there is no reasonable other
way to get at the same problems otherwise, but it of course would be a lot nicer
to have a testcase that is valid C :-)

> Segher, can you suggest how to obtain information (from internal GCC structures) > about such cases ?

I would do something scan-assembler.  I have no idea what instructions accessing
the frame look like on AVR, but I imagine it won't be too hard to see this?

But I know nothing that can be (or *could* be, even) done that is not
target-specific.  But this testcase is in gcc.target/ anyway, right?

(In reply to Segher Boessenkool from comment #13)
> But this testcase is in gcc.target/ anyway, right?
That's just a copy of  gcc.dg/torture/pr64088.c :

https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/testsuite/gcc.dg/torture/pr64088.c;h=0ea5fabcc2fc30833d9ab80ce30090f23e46594b;hb=HEAD

This test case works with avr+reload but it fails with avr+lra, so it appeared to be lra related.

As gcc.dg/torture/pr64088.c is not target-specific, it's supposed to make sense on all targets? And with lra too, of course.  No?

(In reply to Georg-Johann Lay from comment #14)
> (In reply to Segher Boessenkool from comment #13)
> > But this testcase is in gcc.target/ anyway, right?
> That's just a copy of  gcc.dg/torture/pr64088.c :
> 
> https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/testsuite/gcc.dg/torture/
> pr64088.c;h=0ea5fabcc2fc30833d9ab80ce30090f23e46594b;hb=HEAD

Yeah, that is clearly not valid C at all already.

> This test case works with avr+reload but it fails with avr+lra, so it
> appeared to be lra related.
> 
> As gcc.dg/torture/pr64088.c is not target-specific, it's supposed to make
> sense on all targets? And with lra too, of course.  No?

It makes sense never, not on any target, not with LRA nor without.  It is
incorrect according to 6.7.6.2/1 already ("If the expression is a constant
expression, it shall have a value greater than zero."), but dereferencing a
non-existing object isn't correct either, obviously.

(In reply to Segher Boessenkool from comment #15)
> It makes sense never, not on any target, not with LRA nor without.
Though there are test cases that are UB and valid as I just
learned some weeks ago.  When I came across gcc.dg/signbit-6.c

https://gcc.gnu.org/pipermail/gcc/2024-October/244903.html

Quote:

> > So as far as I can see, that test has at least 3 bugs?
> > 
> > 1) Missing -fwrapv so that -INT_MIN is no more UB, hence the
> > test would assert that a[0] == b[0].
> 
> That's not a bug.

This is the test case:

https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/testsuite/gcc.dg/signbit-6.c;h=3a522893222d067eb68e242e48789b2f919fbebb;hb=HEAD#l40

Line 40 ff. lead to -INT_MIN  *without*  -fwrapv or similar.

> 41 /* This will invoke UB due to -INT32_MIN.  The test is supposed to pass
> 42    because GCC is supposed to handle this UB case in a predictable way.  */
> 43 a[0] = INT32_MIN;
> 44 b[0] = INT32_MIN;
 
That comment was added by myself.  According to Tamar, this UB test case was explicitly requestet during the review for some new feature / new optimization.

> The INT_MIN case was added because it was a concern during review.

https://gcc.gnu.org/pipermail/gcc/2024-October/244905.html

To make a long story short, there are cases where GCC is handling UB in a predictable / testable way.  Similar should be achievable with the test case in the current PR:

--------------------------------------------------------

The frame size is known at compile time, and the compiler / LRA is accessing a location outside that area with an addressing mode (SP+const or FP+const) that could easily be checked and diagnosed.

Now as a user, I would prefer current LRA behaviour (ice-on-invalid-code) over Reload's (everything seems to be fine).  But on the other hand, ICEs don't look very professional...

> It makes sense never, not on any target, not with LRA nor without.

So ice-on-invalid-code then.

> It is incorrect according to 6.7.6.2/1 already ("If the expression is a constant
> expression, it shall have a value greater than zero.")

Arrays of size zero are GNU-C addition.  But as far as I understand, when a zero-sized array doesn't occur in a structure, or when it is the only element in a struct, then accesses are invalid.

https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html

(In reply to Georg-Johann Lay from comment #16)
> (In reply to Segher Boessenkool from comment #15)
> > It makes sense never, not on any target, not with LRA nor without.
> Though there are test cases that are UB and valid as I just
> learned some weeks ago.

But I am talking about *this* case, not about UB in general.

> To make a long story short, there are cases where GCC is handling UB in a
> predictable / testable way.  Similar should be achievable with the test case
> in the current PR:

Hopefully.  But the testcase as written is simply non-sensical.  A better
testcase would be nicer.

> But on the other hand, ICEs don't look very professional...

Oh, I think most ICEs look very serious!  :-)

> > It makes sense never, not on any target, not with LRA nor without.
> 
> So ice-on-invalid-code then.

Yup.

> > It is incorrect according to 6.7.6.2/1 already ("If the expression is a constant
> > expression, it shall have a value greater than zero.")
> 
> Arrays of size zero are GNU-C addition.

Most testcases are also compiled without GNU extensions.

> But as far as I understand, when a
> zero-sized array doesn't occur in a structure, or when it is the only
> element in a struct, then accesses are invalid.
> 
> https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html

You can have zero-length arrays, but you cannot have any accesses to
non-existent objects (like any member of a length zero static array).