Patch for c/13801

Joseph S. Myers jsm@polyomino.org.uk
Tue Aug 3 11:21:00 GMT 2004


On Mon, 2 Aug 2004, Zack Weinberg wrote:

> Consider 
> 
> file1.c {
>   int n[5] = { 1, 2, 3, 4, 5 };
> }
> file2.c {
>   extern int n[];
> 
>   int foo() { return sizeof(n); }
> }

Works fine, i.e. the bad sizeof is diagnosed.  The type in the external
scope is the composite type, used for checks of consistency, while the
visible type in the file scope is that based on declarations in the file
scope.

> So you gotta handle IMA.  What does your patch do with the above?  I
> can certainly see how one might get this right in the framework of
> your patch, but it's not clear to me that you considered this.

It works fine.  We do of course need an IMA testsuite harness anyway.

> I don't like these "should"s and "would be nice"s.  Not encountering
> any problems in the test suite is probably an indicator that none of
> the above scenarios occur in the test suite - I would recommend you
> write the code to generate the appropriate diagnostics, and then write
> additional test cases to make sure they all trigger and don't crash
> afterward.  Note that problems in this domain have historically showed
> up in the debug info generators - test all of the possible debug
> formats before saying there's no bugs.

I will add testcases and diagnostics for these cases, but I'm not claiming
no bugs, simply no known bugs and that the cases where you claim bugs
might occur, though possible, are themselves hypothetical, and that
diagnosing these cases is a new feature logically separate from fixing bug
13801.

> There's at least one scenario you didn't consider, which is where a
> variable has incomplete type because it's declared using a struct tag
> that has only been defined in an inner scope, e.g.
> 
>   struct foo;
>   function() { struct foo { int a, int b; }; }
>   struct foo array[5];

Works fine (i.e. is diagnosed); the struct foo in the inner scope is a
different type from that in the outer scope.  However I see this isn't
covered by c99-tag-1.c which I added when fixing tag handling (and finding
the problem described in DR#251 in the process), so I'll add testcases.

> Currently we get this right but only by accident, and I suspect
> further that IMA nightmares are lurking in this area (the struct-tag
> handling code in c-decl.c is mostly untouched by my rewrite).

We know about the IMA problems with structs.  I prepared the analysis
requested by Mark <http://www.srcf.ucam.org/~jsm28/gcc/pre-dr-1.txt>.  
There have been no UK C Panel meetings since then to discuss whether there
are indeed defects (agreed as such by the panel, and understood adequately
by Francis Glassborow as the person who then needs to put the case to the
WG14 meeting for why they are defects) there to submit as one or more DRs
to WG14.

But such problems seem entirely independent of this patch.

> So I ran the numbers (paired two-tailed t-test) and indeed I get about
> 2% probability of its being a chance effect.  Fine.  BUT YOU SHOULD
> HAVE DONE THAT.  I do not appreciate having to do the analysis for
> you.  (If you did do this analysis, I apologize for shouting - but why
> the hell didn't you say so the first time?)

The observed universal practice is that performance figures are given
without any statistical analysis.  I queried this once
<http://gcc.gnu.org/ml/gcc/2001-05/msg01439.html> then gave up and just
followed the existing practice.  I do not doubt that programs in contrib
to measure compile-time and memory performance when running on a given
test set, and to give a report on the figures and their significance,
could be of use if generally used.  (In any case, I was only really
concerned that there was no statistical loss of performance as
justification for the memory usage in this case not being particularly
important; the gain of performance was an unexpected bonus.)

> I don't disagree with any of this in principle - but I do continue to
> say that you can't willy-nilly reduce the amount of information
> carried by the type of a decl, even if you are going to put it back
> before end of file.  Not unless you are prepared to do a total
> decouple of the C front-end from the next stage in the pipeline, and
> by that I mean the next stage gets handed one pointer to a data
> structure representing the entire compilation unit, after the
> front-end is completely done.  We are closer to that than we used to
> be but we are still nowhere near it.  As long as the
> language-independent compiler gets given decl nodes before the
> front-end is done, your approach is not safe.

The same could quite plausibly be said of increasing the information, not
just decreasing it, there's no intrinsic reason why completing a decl's
type should be safer than restoring a visible incomplete type.  But the
problems seem entirely hypothetical and unaddressable without actual bug
reports.

In any case, changes in that direction such as making -funit-at-a-time on
always for C seem entirely independent of this patch and best done
separately.

> You may be wondering why I'm being so hard on you and this patch, and
> the reason is I spent a year and a half of my life coming up with what
> I consider to be the least-bad implementation of C declaration
> handling that's feasible in the present state of the compiler, and I
> still get flak because it's not perfect.  And anything that goes wrong

And I'm trying to improve it by fixing bugs rather than giving flak for
them or expecting you to fix everything to do with c-decl.c.  However the
actual testcases you give work fine with the patch and the suggested
problems are with hypothetical code not present in bootstrap or testsuite
run.

-- 
Joseph S. Myers               http://www.srcf.ucam.org/~jsm28/gcc/
    jsm@polyomino.org.uk (personal mail)
    jsm28@gcc.gnu.org (Bugzilla assignments and CCs)



More information about the Gcc-patches mailing list