[cfe-dev] "Fixes" for two crashes, rant on Tok.getIdentifierInfo() and two more bugs

[Nico Weber]

Hi,

the crash I reported and fixed earlier ( http://lists.cs.uiuc.edu/pipermail/cfe-dev/2007-December/000745.html 
  ) happened because `Tok.getIdentifierInfo()` sometimes returns 0.

These conditions are not clearly documented in Token.h, and even if it  
was documented functions that may or may not return 0 are generally  
error prone. So I grepped clang for calls to `getIdentifierInfo()`. I  
found two places where this function was not handled correctly. Tests  
to reproduce the crashes and makeshift patches are attached (Someone  
familiar with the code needs to look at the FIXMEs in the patch.  
Problems where related to ObjC's @try/@catch and ObjC2 @interface  
prefixes).

(Why is it a good idea to treat stuff like @try as two tokens instead  
of one?)

Furthermore, I'd suggest to at least use an assert if you know that  
`getIdentifierInfo()` can't return 0 and rely on it. Doing an  
`assert(Tok.getIdentifierInfo() && "foo always has ident info")`  
serves as good documentation.


In the following places it was not immediately clear to me why the  
code is valid and `getIdentifierInfo` can't possibly return 0 (line  
numbers relative to rev 45360):

Lex/MacroExpander.cpp:
line 324

Lex/Preprocessor.cpp:
2222
2253
2329

Parse/ParseDecl.cpp:
101
1467

Parse/ParseExpr.cpp:
216
247
(785)

Parse/Parser.cpp:
377 (one of the bugs, fixed with the patch

Parse/ParseObjc.cpp:
304
325 (but only because of strange identation because of tabs instead of  
spaces -- fixed in the attached patch as well)
(476)
1130 (one of the bugs, fixed with the patch)
1164 (one of the bugs, fixed with the patch)
1235

Even better than adding asserts in these lines is to catch this  
problem with the compiler (for example, by putting  
`getIdentifierInfo()` in a subclass and never let it return 0. Then  
you _have_ to check for the right token type to call the method), but  
that's a bit of work :-P



An unrelated crash that I found on the way is:

     int main()
     {
       id a;
       [a bla:0 6:7];
     }

(crashes somewhere in sema, something like this should be put in test/ 
Parse/objc-messaging-1.m)


And here's an inconsistency with gcc:

     int @interface bla ;  // ?? this is valid objc?
     @end

I have no idea what this code is supposed to do, but it doesn't warn  
with clang but doesn't even compile with gcc.

Nico

ps: I also converted a few tabs to spaces

-------------- next part --------------
A non-text attachment was scrubbed...
Name: crashes.patch
Type: application/octet-stream
Size: 7246 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20071226/d630f3b8/attachment.obj>
-------------- next part --------------