This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Fix pr61848, linux kernel miscompile
- From: Alan Modra <amodra at gmail dot com>
- To: "Joseph S. Myers" <joseph at codesourcery dot com>
- Cc: gcc-patches at gcc dot gnu dot org, Andrew Pinski <pinskia at gmail dot com>, Jan Hubicka <hubicka at ucw dot cz>
- Date: Thu, 18 Sep 2014 00:36:54 +0930
- Subject: Re: Fix pr61848, linux kernel miscompile
- Authentication-results: sourceware.org; auth=none
- References: <20140915040242 dot GG17693 at bubble dot grove dot modra dot org> <Pine dot LNX dot 4 dot 64 dot 1409152036000 dot 6402 at digraph dot polyomino dot org dot uk> <20140916060313 dot GH17693 at bubble dot grove dot modra dot org> <Pine dot LNX dot 4 dot 64 dot 1409161245250 dot 7616 at digraph dot polyomino dot org dot uk>
On Tue, Sep 16, 2014 at 01:03:50PM +0000, Joseph S. Myers wrote:
> The point of testsuite additions is to verify the visible changes in
Understood. A long time ago I worked on mil-spec systems..
> > Come to think of it, what if I decline to make any testsuite
> > additions?
>
> Then the patch isn't ready for review. Documentation and testcases are
> the first thing I look at when reviewing C front-end changes; the
> testcases are the primary evidence that the patch does what it's meant to
> do, and without them I won't generally try to review the code changes.
I gave you two testcases. I happen to know how nice it is to have
testcases when reviewing patches. Yes, they weren't testsuite
patches.
Now I'd normally just grumble a bit to myself and comply with a
request for a testsuite patch, but I'm a little annoyed. The
testsuite isn't my personal itch.
I could have just bisected until finding Jan's patch, and asked him
nicely to please fix his breakage. This is what most people do, and
pointing the finger at a particular patch is useful. I went further
and analysed exactly why the patch was at fault. I could have stopped
there. The breakage wasn't really affecting me, and it wasn't my bug
after all.
Instead I thought I'd have a go at fixing the problem, without
bothering Jan: The loss of section attribute looked quite easy to
fix. So I threw together a patch, verified it worked, then knowing
that there is more than one merge_decls in the gcc source code, fixed
the same problem for C++. At that point I found Andrew's bugzilla.
Andrew is competent, I could have just attached the patch I had to
his bugzilla and left him to it.
However it occurred to me that Jan's changes might have broken
something else, so I poked around and found some other fields that had
been moved to symtab_node. No big deal, I fixed that breakage as
well. (I'm not sure I have them all actually. symtab_node fields and
varpool_node fields are not clearly demarcated into those private to
cgraph and those that matter externally.) I could have stopped there
too.
The final piece was wondering why Jan had added an extra test on
DECL_SECTION, seeing that gcc has not reported an error on a changed
section attribute for a very long time, and noticing that tls model
wasn't generally merged. This is obviously where I went wrong. Silly
me. I should have just pointed out these problems and asked you as C
front end maintainer to please think about fixing them. That way you
would have written the patch, and presumably the testsuite, and
everyone would be happy!
In fact, maybe I should have just avoided all this effort and just
asked Jan to please fix his bug.
BTW, in my patch submission at
https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01146.html
I said "trunk does not honour the last section attribute". What I
really should have said was "trunk, with a straight-forward fix for
complete loss of section attribute, does not honour the last section
attribute".
--
Alan Modra
Australia Development Lab, IBM