| Message ID | d9c9572d-b417-0588-36b3-1e4c19cbcaaf@gjlay.de |
|---|---|
| State | New |
| Headers | show |
On 10/26/2016 04:46 PM, Georg-Johann Lay wrote: > + if { [istarget avr-*-*] } { > + # If the value of a label does not fit into 16 bits, the linker > + # will generate a stub (containing a direct jump) and we end up > + # with the address of the stub instead of the address of the very > + # label. Whereas it is legitimate to use such addresses for > + # indirect jumps, it makes no sense to perform any arithmetic > + # on such addresses. > + return [check_no_compiler_messages label_offsets assembly { > + #ifdef __AVR_3_BYTE_PC__ > + #error NO > + #endif > + }] > + } > + return 1; I'm not sure I understand the failure mode. Sure, you're not getting the address of the actual label, but the address of one that's equivalent - so why can't you do arithmetic on it? Where does it go wrong? Am I right in thinking that only the execution test actually fails? Bernd
On 26.10.2016 18:51, Bernd Schmidt wrote: > On 10/26/2016 04:46 PM, Georg-Johann Lay wrote: >> + if { [istarget avr-*-*] } { >> + # If the value of a label does not fit into 16 bits, the linker >> + # will generate a stub (containing a direct jump) and we end up >> + # with the address of the stub instead of the address of the very >> + # label. Whereas it is legitimate to use such addresses for >> + # indirect jumps, it makes no sense to perform any arithmetic >> + # on such addresses. >> + return [check_no_compiler_messages label_offsets assembly { >> + #ifdef __AVR_3_BYTE_PC__ >> + #error NO >> + #endif >> + }] >> + } >> + return 1; > > I'm not sure I understand the failure mode. Sure, you're not getting the > address of the actual label, but the address of one that's equivalent - so why > can't you do arithmetic on it? Where does it go wrong? There are devices where the address of a label does not fit into 16 bits, which is a problem for indirect calls because not all of the program memory can be targeted by 16 bits. The solution was to emit taking address of label LAB as gs(LAB) and let the linker resolve this ("gs" stands for "generate stub"). If LAB actually fits in 16 bits nothing special happens and we have LAB = gs(LAB). If LAB doesn't fit then the linker generates a stub like: gs_LAB: jump LAB and resolves gs_LAB = gs(LAB). gs_LAB must be located in the lower part of the program memory so that gs_LAB will fit into 16 bits. An indirect call / jump then is 1) take address of LAB which returns gs_LAB and 2) jump or call that address which jumps or calls gs_LAB which then jumps to LAB. This works because absolute jumps and calls can target all of the program memory. Moreover comparing gs() labels against each other or against 0 for (un)equality will also work as expected. 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. > Am I right in thinking that only the execution test actually fails? > > Bernd Some of the run tests are failing, but not all of them. This is because gs() depends on code size and location. But there are also some compile tests that fail as avr_print_operand_address tries to warn about such code with "pointer offset from symbol maybe incorrect". Johann
On 10/27/2016 12:16 PM, Georg-Johann Lay 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. Ok, so you can't expect to use the value directly, but I think this is not too different from other targets. You still should be able to add the difference back to &&LAB1 and expect to get &&LAB2 or gs(&&LAB2). That's what the execution test does - why isn't this working? Bernd
On 27.10.2016 12:49, Bernd Schmidt wrote: > On 10/27/2016 12:16 PM, Georg-Johann Lay 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. > > Ok, so you can't expect to use the value directly, but I think this is not too > different from other targets. You still should be able to add the difference > back to &&LAB1 and expect to get &&LAB2 or gs(&&LAB2). That's what the > execution test does - why isn't this working? > > Bernd Makes sense what you are writing... Just replacing a label should not do any harm (except for ordering). I had a deeper look into it and some tests are failing because of a missing Binutils support for special expressions, not because the tests don't work per se. Patching the generated asm to results as would be provided by working Binutils make the test pass. Johann
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 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 implementation detail of the port, the point is the semantics are fixed and constant and useful. In deed that there is even a stub is a private implementation detail of the port. I think the `extra' helpful warning from avr_print_operand_address is wrong and should be removed. Think of the label as gs(LAB), not LAB, burn LAB from your mind. Once you do that, you see you can't talk about the order LAB1 > LAB2, the concept doesn't make any sense. The _only_ think you can talk about is gs(LAB1) > gs(LAB2). And because of that, it is always consistent and works just fine. Once that misguided complains from gcc and bisutils are fixed, are their any failing cases?
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... 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. If a dg-require is not appropriate, should I rather add dg-skip-if to every test case? I'd still prefer the dg-require solution because if the Binutils will ever support label differences, then the testsuite will automatically catch up with that. > 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. > implementation detail of the port, the point is the semantics are fixed and > constant and useful. In deed that there is even a stub is a private > implementation detail of the port. I think the `extra' helpful warning from > avr_print_operand_address is wrong and should be removed. Think of the The following code simple makes absolutely no sense in the presence of stubs: int test (int foo) { static void *dummy[] = { &&a, &&b }; goto *((char *) &&b - 2 * (foo < 0)); a: b: return 0; } It's only a compile test (the original PR66123 is about ICE), but the compiler cannot know that it's just a crazy test case that won't be executed... When you are offsetting from a stub you might end up *anywhere*, even in some data range. > label as gs(LAB), not LAB, burn LAB from your mind. Once you do that, you > see you can't talk about the order LAB1 > LAB2, the concept doesn't make any > sense. The _only_ think you can talk about is gs(LAB1) > gs(LAB2). And > because of that, it is always consistent and works just fine. > > Once that misguided complains from gcc and bisutils are fixed, are their any > failing cases? State of matters is that Binutils support is missing, and if I understand you correctly, dg-require is not appropriate to factor out availability of such features? Johann
Georg-Johann Lay writes: > 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... > > 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. > > If a dg-require is not appropriate, should I rather add dg-skip-if to every > test case? I'd still prefer the dg-require solution because if the Binutils > will ever support label differences, then the testsuite will automatically > catch up with that. > >> 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. > >> implementation detail of the port, the point is the semantics are fixed and >> constant and useful. In deed that there is even a stub is a private >> implementation detail of the port. I think the `extra' helpful warning from >> avr_print_operand_address is wrong and should be removed. Think of the > > The following code simple makes absolutely no sense in the presence of stubs: > int > test (int foo) > { > static void *dummy[] = { &&a, &&b }; > goto *((char *) &&b - 2 * (foo < 0)); > a: > b: > return 0; > } > > It's only a compile test (the original PR66123 is about ICE), but the compiler > cannot know that it's just a crazy test case that won't be executed... > > When you are offsetting from a stub you might end up *anywhere*, even in some > data range. > >> label as gs(LAB), not LAB, burn LAB from your mind. Once you do that, you >> see you can't talk about the order LAB1 > LAB2, the concept doesn't make any >> sense. The _only_ think you can talk about is gs(LAB1) > gs(LAB2). And >> because of that, it is always consistent and works just fine. >> >> Once that misguided complains from gcc and bisutils are fixed, are their any >> failing cases? > > State of matters is that Binutils support is missing, and if I understand you > correctly, dg-require is not appropriate to factor out availability of such > features? I'll take a stab at adding the missing binutils support for avr label diffs. Regards Senthil
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 04.11.2016 06:18, Senthil Kumar Selvaraj wrote: > > Georg-Johann Lay writes: >> State of matters is that Binutils support is missing, and if I understand you >> correctly, dg-require is not appropriate to factor out availability of such >> features? > > I'll take a stab at adding the missing binutils support for avr label diffs. Thanks for taking care of this. I had a look into it but got stuck with a test case derived from gcc.c-torture/execute/pr70460.c int c; __attribute__((noinline, noclone)) void call (void) { __asm ("nop"); } __attribute__((noinline, noclone)) void foo (int x) { static int b[] = { &&lab1 - &&lab0, &&lab2 - &&lab0 }; void *a = &&lab0 + b[x]; goto *a; lab1: c += 2; call(); lab2: c++; lab0: ; } int main () { foo (0); if (c != 3) __builtin_abort (); foo (1); if (c != 4) __builtin_abort (); return 0; } The problem is when relaxation is turned on and the CALL is shortened to RCALL but respective literals are not fixed up. Johann > Regards > Senthil
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.
Georg-Johann Lay writes: > On 04.11.2016 06:18, Senthil Kumar Selvaraj wrote: >> >> Georg-Johann Lay writes: >>> State of matters is that Binutils support is missing, and if I understand you >>> correctly, dg-require is not appropriate to factor out availability of such >>> features? >> >> I'll take a stab at adding the missing binutils support for avr label diffs. > > Thanks for taking care of this. I had a look into it but got stuck with a test > case derived from gcc.c-torture/execute/pr70460.c > > int c; > > __attribute__((noinline, noclone)) void > call (void) > { > __asm ("nop"); > } > > __attribute__((noinline, noclone)) void > foo (int x) > { > static int b[] = { &&lab1 - &&lab0, &&lab2 - &&lab0 }; > void *a = &&lab0 + b[x]; > goto *a; > lab1: > c += 2; > call(); > lab2: > c++; > lab0: > ; > } > > int > main () > { > foo (0); > if (c != 3) > __builtin_abort (); > foo (1); > if (c != 4) > __builtin_abort (); > return 0; > } > > > The problem is when relaxation is turned on and the CALL is shortened to RCALL > but respective literals are not fixed up. That linker issue is fixed with https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=4cb771f214ed6a2102e37bce255c6be5d0642f3a Regards Senthil
Index: gcc.c-torture/compile/20021108-1.c =================================================================== --- gcc.c-torture/compile/20021108-1.c (revision 241546) +++ gcc.c-torture/compile/20021108-1.c (working copy) @@ -1,4 +1,4 @@ -/* { dg-require-effective-target label_values } */ +/* { dg-require-effective-target label_offsets } */ int main() Index: gcc.c-torture/compile/920501-7.c =================================================================== --- gcc.c-torture/compile/920501-7.c (revision 241546) +++ gcc.c-torture/compile/920501-7.c (working copy) @@ -1,3 +1,3 @@ -/* { dg-require-effective-target label_values } */ +/* { dg-require-effective-target label_offsets } */ x(){if(&&e-&&b<0)x();b:goto*&&b;e:;} Index: gcc.c-torture/compile/labels-2.c =================================================================== --- gcc.c-torture/compile/labels-2.c (revision 241546) +++ gcc.c-torture/compile/labels-2.c (working copy) @@ -1,4 +1,4 @@ -/* { dg-require-effective-target label_values } */ +/* { dg-require-effective-target label_offsets } */ struct bp { void *v, *b, *e; }; f () Index: gcc.c-torture/compile/labels-3.c =================================================================== --- gcc.c-torture/compile/labels-3.c (revision 241546) +++ gcc.c-torture/compile/labels-3.c (working copy) @@ -1,6 +1,6 @@ /* Verify that we can narrow the storage associated with label diffs. */ /* { dg-require-effective-target indirect_jumps } */ -/* { dg-require-effective-target label_values } */ +/* { dg-require-effective-target label_offsets } */ int foo (int a) { Index: gcc.c-torture/execute/pr70460.c =================================================================== --- gcc.c-torture/execute/pr70460.c (revision 241546) +++ gcc.c-torture/execute/pr70460.c (working copy) @@ -1,5 +1,5 @@ /* { dg-require-effective-target indirect_jumps } */ -/* { dg-require-effective-target label_values } */ +/* { dg-require-effective-target label_offsets } */ /* PR rtl-optimization/70460 */ Index: gcc.dg/20021029-1.c =================================================================== --- gcc.dg/20021029-1.c (revision 241546) +++ gcc.dg/20021029-1.c (working copy) @@ -3,7 +3,7 @@ /* { dg-do compile { target fpic } } */ /* { dg-options "-O2 -fpic" } */ /* { dg-final { scan-assembler-not ".data.rel.ro.local" } } */ -/* { dg-require-effective-target label_values } */ +/* { dg-require-effective-target label_offsets } */ /* { dg-require-effective-target indirect_jumps } */ int foo (int a) Index: gcc.dg/pr16973.c =================================================================== --- gcc.dg/pr16973.c (revision 241546) +++ gcc.dg/pr16973.c (working copy) @@ -3,7 +3,7 @@ to add back the label. */ /* { dg-options "" } */ -/* { dg-require-effective-target label_values } */ +/* { dg-require-effective-target label_offsets } */ void f (void) Index: gcc.dg/torture/pr66123.c =================================================================== --- gcc.dg/torture/pr66123.c (revision 241546) +++ gcc.dg/torture/pr66123.c (working copy) @@ -1,5 +1,5 @@ /* { dg-do compile } */ -/* { dg-require-effective-target label_values } */ +/* { dg-require-effective-target label_offsets } */ int test (int foo) Index: gcc.dg/torture/pr66178.c =================================================================== --- gcc.dg/torture/pr66178.c (revision 241546) +++ gcc.dg/torture/pr66178.c (working copy) @@ -1,5 +1,5 @@ /* { dg-do compile } */ -/* { dg-require-effective-target label_values } */ +/* { dg-require-effective-target label_offsets } */ int test(void) { Index: lib/target-supports.exp =================================================================== --- lib/target-supports.exp (revision 241546) +++ lib/target-supports.exp (working copy) @@ -740,6 +740,31 @@ proc check_effective_target_label_values }] } +# Return 1 if offsetting label values is supported, 0 otherwise. +# A typical offset is the value of a different label like in +# &&lab1 - &&lab2. + +proc check_effective_target_label_offsets {} { + # Offsetting labels implies we can take values of labels. + if { ![check_effective_target_label_values] } { + return 0; + } + if { [istarget avr-*-*] } { + # If the value of a label does not fit into 16 bits, the linker + # will generate a stub (containing a direct jump) and we end up + # with the address of the stub instead of the address of the very + # label. Whereas it is legitimate to use such addresses for + # indirect jumps, it makes no sense to perform any arithmetic + # on such addresses. + return [check_no_compiler_messages label_offsets assembly { + #ifdef __AVR_3_BYTE_PC__ + #error NO + #endif + }] + } + return 1; +} + # Return 1 if builtin_return_address and builtin_frame_address are # supported, 0 otherwise.