diff mbox

breakage with "[PATCH 1/6] Add FOR_EACH_INSN{_INFO}_{DEFS,USES,EQ_USES}"

Message ID CABu31nNNu+zuywbTjP+=3h99LWcQuGPP60qqQvG0R3dQAWFTWw@mail.gmail.com
State New
Headers show

Commit Message

Steven Bosscher June 15, 2014, 4:21 p.m. UTC 
On Sun, Jun 15, 2014 at 1:27 PM, Hans-Peter Nilsson wrote:
> On Sat, 14 Jun 2014, Richard Sandiford wrote:
>
>> To make the final representation change easier, this patch introduces
>> macros for iterating over lists of defs, uses and eq_uses.  At the
>> moment there are three possible keys when accessing df_ref lists:
>> the insn rtx (DF_INSN_*), the insn uid (DF_INSN_UID_*) and the
>> df_insn_info (DF_INSN_INFO_*).  I don't think it's worth adding
>> iterators for uids though.  Any code that's going to the trouble of
>> caching the uid might as well go the whole hog and cache the underlying
>> df_insn_info.
>>
>> After the feedback to the BB iterator patch:
>>
>>   https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00676.html
>>
>> I've kept the iterator variable definitions outside the FOR_* macros
>> rather than make the FOR_* macros define the variables themselves.
>>
>> Richard
>>
>>
>> gcc/
>>       * df.h (DF_INSN_INFO_MWS, FOR_EACH_INSN_INFO_DEF): New macros.
>>       (FOR_EACH_INSN_INFO_USE, FOR_EACH_INSN_INFO_EQ_USE): Likewise.
>>       (FOR_EACH_INSN_DEF, FOR_EACH_INSN_USE, FOR_EACH_INSN_EQ_USE): Likewise.
>>       * auto-inc-dec.c (find_inc, merge_in_block): Use them.
>
>
> One of these patches (in 211677:211684) broke cris-elf:
>
> g++ -c   -g -O2 -DIN_GCC  -DCROSS_DIRECTORY_STRUCTURE
> -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall
> \
> -Wwrite-strings -Wcast-qual -Wmissing-format-attribute
> -Woverloaded-virtual -pedantic -Wno-long-long
> -Wno-variadic-macr\
> os -Wno-overlength-strings -fno-common  -DHAVE_CONFIG_H -I. -I.
> -I/tmp/hpautotest-gcc0/gcc/gcc -I/tmp/hpautotest-gcc0/g\
> cc/gcc/. -I/tmp/hpautotest-gcc0/gcc/gcc/../include
> -I/tmp/hpautotest-gcc0/gcc/gcc/../libcpp/include
> -I/tmp/hpautotest-g\
> cc0/cris-elf/gccobj/./gmp -I/tmp/hpautotest-gcc0/gcc/gmp
> -I/tmp/hpautotest-gcc0/cris-elf/gccobj/./mpfr -I/tmp/hpautotes\
> t-gcc0/gcc/mpfr -I/tmp/hpautotest-gcc0/gcc/mpc/src
> -I/tmp/hpautotest-gcc0/gcc/gcc/../libdecnumber
> -I/tmp/hpautotest-gc\
> c0/gcc/gcc/../libdecnumber/dpd -I../libdecnumber
> -I/tmp/hpautotest-gcc0/gcc/gcc/../libbacktrace    -o
> auto-inc-dec.o -M\
> T auto-inc-dec.o -MMD -MP -MF ./.deps/auto-inc-dec.TPo
> /tmp/hpautotest-gcc0/gcc/gcc/auto-inc-dec.c
> /tmp/hpautotest-gcc0/gcc/gcc/auto-inc-dec.c: In function 'void
> merge_in_block(int, basic_block_def*)':
> /tmp/hpautotest-gcc0/gcc/gcc/auto-inc-dec.c:1442: error: 'uid'
> was not declared in this scope
> make[2]: *** [auto-inc-dec.o] Error 1
>
> brgds, H-P


Bah, this is why I just *hate* all the gcc code that's only compiled
if certain #defines are set...

Can you please try:


Ciao!
Steven

Comments

Hans-Peter Nilsson June 15, 2014, 10:36 p.m. UTC | #1 
On Sun, 15 Jun 2014, Steven Bosscher wrote:
> On Sun, Jun 15, 2014 at 1:27 PM, Hans-Peter Nilsson wrote:
> > /tmp/hpautotest-gcc0/gcc/gcc/auto-inc-dec.c: In function 'void
> > merge_in_block(int, basic_block_def*)':
> > /tmp/hpautotest-gcc0/gcc/gcc/auto-inc-dec.c:1442: error: 'uid'
> > was not declared in this scope
> > make[2]: *** [auto-inc-dec.o] Error 1
> >
> > brgds, H-P
>
>
> Bah, this is why I just *hate* all the gcc code that's only compiled
> if certain #defines are set...

I couldn't agree more.  Might not have been obvious when writing
the mosly-mechanical patch, still the auto-inc-dec.c name should
have been a red flag that a representative target should have
been tested (i.e. not x86_64 and i686).

>
> Can you please try:
>
> [...]

Thanks.  Looks pretty obvious.  I was heading for the door with
just enough time to report the issue, so I didn't actually look
at the code before.  I'll commit this on your behalf once build
has passed the point of failure.

brgds, H-P
Hans-Peter Nilsson June 15, 2014, 10:52 p.m. UTC | #2 
On Sun, 15 Jun 2014, Hans-Peter Nilsson wrote:
> On Sun, 15 Jun 2014, Steven Bosscher wrote:
> > Can you please try:
> >
> > [...]
>
> Thanks.  Looks pretty obvious.  I was heading for the door with
> just enough time to report the issue, so I didn't actually look
> at the code before.  I'll commit this on your behalf once build
> has passed the point of failure.

...which includes not just compiling auto-inc-dec.c but also
compiling e.g. libgcc with the compiled compiler.  No such luck,
segv in that function (must be from Richard's code, not from the
patch as no dumps are output at that time).  Bah.

brgds, H-P
Hans-Peter Nilsson June 15, 2014, 11:39 p.m. UTC | #3 
On Sun, 15 Jun 2014, Hans-Peter Nilsson wrote:

> On Sun, 15 Jun 2014, Hans-Peter Nilsson wrote:
> > On Sun, 15 Jun 2014, Steven Bosscher wrote:
> > > Can you please try:
> > >
> > > [...]
> >
> > Thanks.  Looks pretty obvious.  I was heading for the door with
> > just enough time to report the issue, so I didn't actually look
> > at the code before.  I'll commit this on your behalf once build
> > has passed the point of failure.
>
> ...which includes not just compiling auto-inc-dec.c but also
> compiling e.g. libgcc with the compiled compiler.  No such luck,
> segv in that function (must be from Richard's code, not from the
> patch as no dumps are output at that time).  Bah.

Ok, I'm out; I don't know why the insn_info is invalid here.
Hopefully obvious to you or Richard.  PR61516 for this.

brgds, H-P
Steven Bosscher June 16, 2014, 8:29 a.m. UTC | #4 
On Mon, Jun 16, 2014 at 12:36 AM, Hans-Peter Nilsson wrote:
> On Sun, 15 Jun 2014, Steven Bosscher wrote:
>> On Sun, Jun 15, 2014 at 1:27 PM, Hans-Peter Nilsson wrote:
>> > /tmp/hpautotest-gcc0/gcc/gcc/auto-inc-dec.c: In function 'void
>> > merge_in_block(int, basic_block_def*)':
>> > /tmp/hpautotest-gcc0/gcc/gcc/auto-inc-dec.c:1442: error: 'uid'
>> > was not declared in this scope
>> > make[2]: *** [auto-inc-dec.o] Error 1
>> >
>> > brgds, H-P
>>
>>
>> Bah, this is why I just *hate* all the gcc code that's only compiled
>> if certain #defines are set...
>
> I couldn't agree more.  Might not have been obvious when writing
> the mosly-mechanical patch, still the auto-inc-dec.c name should
> have been a red flag that a representative target should have
> been tested (i.e. not x86_64 and i686).

I agree, but I think you'd agree with me if I say that Richard S. is
one of the few people who almost always goes beyond the normal amount
of testing required for a patch. Breakage like this will just happen
to us all, every once in a while, until we compile all middle-end code
at least, regardless of #defines and whatnot (conditionally compiled
code, from the top of my head: CC0, scheduler, dbrsched, auto-inc-dec,
HAVE_conditional_move, etc...).

Ciao!
Steven
Hans-Peter Nilsson June 16, 2014, 11:23 a.m. UTC | #5 
On Mon, 16 Jun 2014, Steven Bosscher wrote:
> On Mon, Jun 16, 2014 at 12:36 AM, Hans-Peter Nilsson wrote:
> > On Sun, 15 Jun 2014, Steven Bosscher wrote:
> >> On Sun, Jun 15, 2014 at 1:27 PM, Hans-Peter Nilsson wrote:
> >> > /tmp/hpautotest-gcc0/gcc/gcc/auto-inc-dec.c: In function 'void
> >> > merge_in_block(int, basic_block_def*)':
> >> > /tmp/hpautotest-gcc0/gcc/gcc/auto-inc-dec.c:1442: error: 'uid'
> >> > was not declared in this scope
> >> > make[2]: *** [auto-inc-dec.o] Error 1
> >> >
> >> > brgds, H-P
> >>
> >>
> >> Bah, this is why I just *hate* all the gcc code that's only compiled
> >> if certain #defines are set...
> >
> > I couldn't agree more.  Might not have been obvious when writing
> > the mosly-mechanical patch, still the auto-inc-dec.c name should
> > have been a red flag that a representative target should have
> > been tested (i.e. not x86_64 and i686).
>
> I agree, but I think you'd agree with me if I say that Richard S. is
> one of the few people who almost always goes beyond the normal amount
> of testing required for a patch.

His testing efforts relative many others I'd agree on (and I
seem to have a knack for being caught when things still go bad
with his patches), but I'd say the amount of testing that
Richard S usually applies is entirely appropriate and I wish
others would follow!  Also, looks like he fixed it.  (Thanks!)

> Breakage like this will just happen
> to us all, every once in a while, until we compile all middle-end code
> at least, regardless of #defines and whatnot (conditionally compiled
> code, from the top of my head: CC0, scheduler, dbrsched, auto-inc-dec,
> HAVE_conditional_move, etc...).

The patch looked pretty mechanical.  I feared the rabbit-hole,
but the stage 2 issue was apparently just the use of stale data
from the loop.  Exercising that required actually running the
code when testing.

brgds, H-P
diff mbox

Patch

Index: auto-inc-dec.c
===================================================================
--- auto-inc-dec.c      (revision 211685)
+++ auto-inc-dec.c      (working copy)
@@ -1439,7 +1439,8 @@  merge_in_block (int max_reg, basic_block bb)
            }
        }
       else if (dump_file)
-       fprintf (dump_file, "skipping update of deleted insn %d\n", uid);
+       fprintf (dump_file, "skipping update of deleted insn %d\n",
+                INSN_UID (insn));
     }

   /* If we were successful, try again.  There may have been several