Re: [patch] Fix PR 23439/23440

[Volker Reichelt <reichelt at igpm dot rwth-aachen dot de>]

On 12 Sep, Jeffrey A Law wrote:
> On Sun, 2005-09-11 at 23:16 +0200, Volker Reichelt wrote:
>> The following patch fixes an ice-on-invalid-code of both the C and C++
>> frontend for code like this:
>> 
>>   void foo() { for
>> 
>> In this case both frontends call annotate_with_file_line with a NULL
>> instead of a proper filename. The NULL is passed to strcmp which
>> segfaults.
>> 
>> The patch fixes this by returning early from annotate_with_file_line,
>> if no filename is given. A testcase for each frontend is also included.

[snip]

> Weird -- I would have expected to still have a valid FILE argument
> in these cases.  Can you explain a little why we do not have a valid
> FILE argument?
> 
> jeff

The C++ parser expects a statement (the loop body) after the
parentheses of a for-statement and tries to annotate it.
But in the testcase above this statement isn't there - we already
reached the end of the token stream. That's why we get a NULL
file name.

One could try to fix this in the callers of annotate_with_file_line,
but to be sure that all cases are caught, I'd prefer to fix
annotate_with_file_line.

The only culprits in annotate_with_file_line are the two instances
of strcmp which cannot handle NULL pointers as argument.
So there are a couple of options:

a) Return unconditionally on empty file name (as proposed in the
   patch). This way the "file" argument of strcmp can never be empty.
   The other argument shouldn't be empty, too, since we never call
   perform annotations with empty file names in a_w_f_l. (On second
   thought, however, I'm not quite sure whether some other function
   makes such an annotation.)

b) Guard the two calls to strcmp with argument checks.
   This is bullet-proof, but probably causes more overhead.

c) Just get rid of the calls to strcmp. This is bullet-proof, too.
   We would lose some opportunities for sharing annotations, though.

With my patch http://gcc.gnu.org/ml/gcc-patches/2005-09/msg00679.html
to check line numbers before file names some interesting things
happened:

1) It became more difficult to trigger the segfault:
   We have to generate an annotation with NULL as file name (the
   corresponding line number is zero) and one with an non-NULL file
   name and also line number zero).
   The original testcases do not generate the second annotation
   and therefore pass!
   However I still can generate a segfault if I lie about the line
   numbers like in the following example:

   ======================
   # 0 "bug.cc"
   void foo() { for
   ======================

   We could say that using line number zero is just asking for trouble
   and do not try to fix this at all. (In this case I'd like to apply
   the patch mentioned above to the 4.,0 branch, too.

2) The strcmp now only catches annotation sharing opportunities in code
   that lies about the real line numbers. At least that's what a GCC
   bootstrap suggests: I got non-zero numbers only for the following
   nine files:

     gengtype-lex.c:    ~  30
     gengtype-yacc.c:   ~  30
     insn-attrtab.c:    ~5000
     insn-emit.c:       ~  60
     insn-output.c:     ~  25
     insn-recog.c:      ~  70
     insn-preds.c:      ~  20
     java/parse.c:      ~ 230
     java/parse-scan.c: ~  25

   Apart from insn-attrtab.c the numbers are quite small. This makes
   option c) a viable option IMHO.

I hope this provides some more insight. Should I go ahead with plan a)
as in the original patch, or would you suggest something different?

Regards,
Volker