[testsuite] Support dg-require-effective-target label_offsets.

On Nov 3, 2016, at 8:25 AM, Georg-Johann Lay <avr@gjlay.de> wrote:
> 
> On 28.10.2016 17:58, Mike Stump wrote:
>> On Oct 27, 2016, at 3:16 AM, Georg-Johann Lay <avr@gjlay.de> wrote:
>>> 
>>> Now imagine some arithmetic like &&LAB2 - &&LAB1.  This might result in
>>> one or two stub addresses, and difference between such addresses is a
>>> complete different thing than the difference between the original labels:
>>> The result might differ in absolute value and in sign, i.e. you can't even
>>> determine whether LAB1 or LAB2 comes first in the generated code as the
>>> order of stubs might differ from the order of respective labels.
>> 
>> So, I think this all doesn't matter any.  Every address gs(LAB) fits in
>> 16-bits by definition, and every gs(LAB1) - gs(LAB2) fits into 16 bits and
> 
> Yes, you are right.  Label differences could be implemented.  AFAIK there is currently no activity by the Binutils folks to add respective support, so it is somewhat pointless to add that support to avr-gcc...


[ pause, built an avr compiler ] So, I checked code-gen for gcc.c-torture/compile/labels-2.c and it looked fine:

        ldi r24,lo8(gs(.L2))
        ldi r25,hi8(gs(.L2))
        std Y+2,r25
        std Y+1,r24
        ldi r18,lo8(gs(.L3))
        ldi r19,hi8(gs(.L3))
        ldi r24,lo8(gs(.L4))
        ldi r25,hi8(gs(.L4))
        mov r20,r18
        mov r21,r19
        sub r20,r24

So, apparently quite a lot of the code-gen is already wired up.  Since this generated code, I wasn't sure what error you're trying to hide?  I tried all the test cases your mentioned, none failed to produce code or failed to assemble that code with -mmcu=atmega2560 nor any other cpu I could find, so, trivially, I must not quite understand what doesn't work yet.

I did notice that gcc.c-torture/execute/pr70460.c produced:

.word   .L3-(.L2)

which seems wrong, given all the other code-gen.  I think this has to be gs(.L3)-(gs(.L2)), no?  I tried to get binutils to do that for me directly with a .word and it seemed resistant; which is unfortunate.

If you consider the above to be a code-gen bug, then you can do something like:

to get the compiler to error out to prevent bad code-gen that binutils can't handle.  If you want, you can also declare by fiat that subtraction might be performed on the stub or the actual function, and so any code that does this isn't supported (runtime unspecified behavior), and then you just mark the execute testcase as bad (not supportable on avr, since avr blew the binutils support for gs).  The compile only test cases work fine now, so nothing needs to be done for them.

If you go for the above and make it a hard error, then a patch in the style of the original patch would be fine, but please just mark the ones that fail and please key of avr*, as this is an avr-only misfeature.  If avr did it right, they would complete gs() support so that gs(LAB)-gs(LAB) works.  gcc has a fetish for + and - working with labels and constants.

The completely different solution would be to never use gs(), and generate code that is 3 byte aware, but, I suspect you're unlikely to want to do that.

> Bottom line is that label offsets are not supported by avr BE, and the intention of the patch is to reduce FAIL noise in testsuite results because of the missing support.

So, what doesn't work?  I tried all the tests cases on a top of tree compiler, and nothing I did ever failed.

>> thus is valid for all 16-bit one contexts.  The fact the order between the
>> stub and the actual code is different is irrelevant, it is a private
> 
> Only if the code is not executed; there are several test cases that compute relative placements of labels like:
> 
> x(){if(&&e-&&b<0)x();b:goto*&&b;e:;}
> 
> If a test ever relies on the fact that &&e - &&b tells anything about the ordering of the labels, the code is about to crash.

No, I checked the code, it's perfectly fine, with the one exception that I think .L3 is used in places where gs(.L3) should be used.  The single bug I found is a port bug, and it wants to sometimes use gs() and sometimes not use gs().  You can't do that.  For the totality of what people want to work, you have to either use gs() everywhere, or nowhere; it is the mixture that is the problem.  And then, the only thing that bug prevents is one test case from running.  That test case could be skipped on avr, if it doesn't work reliably, as most other port maintainers would rather be consistent, and if they are, they can't hit this avr port bug.

Also, the code produced isn't run, so arguing what would happen when run is silly, as, that's not the case in any of the tests except one of them.  For the last one, there would appear to be a compiler code-gen bug that needs fixing as mentioned above.

On 04.11.2016 03:48, Mike Stump wrote:
> On Nov 3, 2016, at 8:25 AM, Georg-Johann Lay <avr@gjlay.de> wrote:
>>
>> On 28.10.2016 17:58, Mike Stump wrote:
>>> On Oct 27, 2016, at 3:16 AM, Georg-Johann Lay <avr@gjlay.de> wrote:
>>>>
>>>> Now imagine some arithmetic like &&LAB2 - &&LAB1.  This might result in
>>>> one or two stub addresses, and difference between such addresses is a
>>>> complete different thing than the difference between the original labels:
>>>> The result might differ in absolute value and in sign, i.e. you can't even
>>>> determine whether LAB1 or LAB2 comes first in the generated code as the
>>>> order of stubs might differ from the order of respective labels.
>>>
>>> So, I think this all doesn't matter any.  Every address gs(LAB) fits in
>>> 16-bits by definition, and every gs(LAB1) - gs(LAB2) fits into 16 bits and
>>
>> Yes, you are right.  Label differences could be implemented.  AFAIK there is currently no activity by the Binutils folks to add respective support, so it is somewhat pointless to add that support to avr-gcc...
>
>
> [ pause, built an avr compiler ] So, I checked code-gen for gcc.c-torture/compile/labels-2.c and it looked fine:
>
>         ldi r24,lo8(gs(.L2))
>         ldi r25,hi8(gs(.L2))
>         std Y+2,r25
>         std Y+1,r24
>         ldi r18,lo8(gs(.L3))
>         ldi r19,hi8(gs(.L3))
>         ldi r24,lo8(gs(.L4))
>         ldi r25,hi8(gs(.L4))
>         mov r20,r18
>         mov r21,r19
>         sub r20,r24
>
> So, apparently quite a lot of the code-gen is already wired up.  Since this generated code, I wasn't sure what error you're trying to hide?  I tried all the test cases your mentioned, none failed to produce code or failed to assemble that code with -mmcu=atmega2560 nor any other cpu I could find, so, trivially, I must not quite understand what doesn't work yet.
>
> I did notice that gcc.c-torture/execute/pr70460.c produced:
>
> .word   .L3-(.L2)
>
> which seems wrong, given all the other code-gen.  I think this has to be gs(.L3)-(gs(.L2)), no?  I tried to get binutils to do that for me directly with a .word and it seemed resistant; which is unfortunate.

Well, the GCC specification on void* arithmetic requires that void is handled 
as if it has a size of one, and .L3-.L2 meets that requirement.  As code 
addresses on avr are word addresses, not byte addresses, the above code will 
crash.  A working expression would be (.L3-.L2)/2 but 1) this conflicts with 
the gcc specification 2) it does not work with stubs 3) the linker does not 
handle this correctly if relaxation is on.

All the label-arithmetic test cases in GCC testsuite don't actually need stub 
generation because the binaries are not big enough.  Using gs, the expression 
would be gs(.L3)-(gs(.L2)) as gs implements word addresses, not byte addresses.

This also means the problem is not only present with 3-byte PC devises but also 
for smaller ones.

>
> If you consider the above to be a code-gen bug, then you can do something like:
>
> Index: config/avr/avr.c
> ===================================================================
> --- config/avr/avr.c    (revision 241837)
> +++ config/avr/avr.c    (working copy)
> @@ -9422,6 +9422,21 @@
>        x = plus_constant (Pmode, x, AVR_TINY_PM_OFFSET);
>      }
>
> +  /* We generate stubs using gs(), but binutils doesn't support that
> +     here.  */
> +  if (AVR_3_BYTE_PC
> +      && GET_CODE (x) == MINUS
> +      && GET_CODE (XEXP (x, 0)) == LABEL_REF
> +      && GET_CODE (XEXP (x, 1)) == LABEL_REF)
> +    return false;
> +  if (AVR_3_BYTE_PC
> +      && GET_CODE (x) == MINUS
> +      && GET_CODE (XEXP (x, 0)) == SUBREG
> +      && GET_CODE (XEXP (XEXP (x, 0), 0)) == LABEL_REF
> +      && GET_CODE (XEXP (x, 1)) == SUBREG
> +      && GET_CODE (XEXP (XEXP (x, 1), 0)) == LABEL_REF)
> +    return false;
> +
>    return default_assemble_integer (x, size, aligned_p);
>  }
>
> to get the compiler to error out to prevent bad code-gen that binutils can't handle.  If you want, you can also declare by fiat that subtraction might be performed on the stub or the actual function, and so any code that does this isn't supported (runtime unspecified behavior), and then you just mark the execute testcase as bad (not supportable on avr, since avr blew the binutils support for gs).  The compile only test cases work fine now, so nothing needs to be done for them.

ICE is not a nice approach, IMO.  I'd prefer some more graceful error using 
code locations of the labels (which are not available).

>
> If you go for the above and make it a hard error, then a patch in the style of the original patch would be fine, but please just mark the ones that fail and please key of avr*, as this is an avr-only misfeature.  If avr did it right, they would complete gs() support so that gs(LAB)-gs(LAB) works.  gcc has a fetish for + and - working with labels and constants.
>
> The completely different solution would be to never use gs(), and generate code that is 3 byte aware, but, I suspect you're unlikely to want to do that.

That's the worst solution of all because it changes the ABI, leads to code 
bloat, and many more issues.  And just reduce testsuite fallout for some crazy 
tests? Really?

On Nov 4, 2016, at 2:35 AM, Georg-Johann Lay <avr@gjlay.de> wrote:
> 
>> .word   .L3-(.L2)
>> 
>> which seems wrong, given all the other code-gen.  I think this has to be gs(.L3)-(gs(.L2)), no?  I tried to get binutils to do that for me directly with a .word and it seemed resistant; which is unfortunate.

> Using gs, the expression would be gs(.L3)-(gs(.L2)) as gs implements word addresses, not byte addresses.

As I said, I tried that and could not get it to work.

>> The compile only test cases work fine now, so nothing needs to be done for them.
> 
> ICE is not a nice approach

I never saw an ICE with current release binutils and top of tree gcc.  If you want me to see an ICE and comment on it, I would need the command line apparently.

> , IMO.  I'd prefer some more graceful error using code locations of the labels (which are not available).

My preference is code-gen.  :-)  Anyway, the change I posted creates nice graceful errors, if one wants.