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