<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Dec 26, 2007, at 10:44 AM, Nico Weber wrote:</div><blockquote type="cite">the crash I reported and fixed earlier ( <a href="http://lists.cs.uiuc.edu/pipermail/cfe-dev/2007-December/000745.html">http://lists.cs.uiuc.edu/pipermail/cfe-dev/2007-December/000745.html</a> ) happened because `Tok.getIdentifierInfo()` sometimes returns 0.</blockquote><div><br class="webkit-block-placeholder"></div><div>Right. Tokens that are not "pp-identifiers" in the lexer do not have an identifier pointer. This includes tokens like numbers (1), strings ("foo"), etc.</div><br><blockquote type="cite">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).</blockquote><div><br class="webkit-block-placeholder"></div>Nice! Your patch looks exactly right, I applied it here (after tweaking the expected-error stuff):</div><div><a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20071224/003579.html">http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20071224/003579.html</a></div><div><br class="webkit-block-placeholder"></div><div><blockquote type="cite">(Why is it a good idea to treat stuff like @try as two tokens instead of one?)</blockquote><div><br class="webkit-block-placeholder"></div><div>The answer is that thing like @ /*comment*/ try are legal, sadly enough. However, it seems that we could probably do something in the lexer (when it sees the "@", to handle this. I'll see what I can do about this when I have time.</div><br><blockquote type="cite">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.</blockquote><div><br class="webkit-block-placeholder"></div><div>Well, in theory the code should only call and deference getIdentifierInfo if it already knows. If it isn't clear from the context of the call in the code, adding an assert makes sense.</div><div><br class="webkit-block-placeholder"></div><blockquote type="cite">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):<br><br>Lex/MacroExpander.cpp:<br>line 324</blockquote><div><br class="webkit-block-placeholder"></div>This is safe because previous code verified that the macro arguments are identifiers.</div><div><br class="webkit-block-placeholder"></div><div>#define A(1) </div><div><br class="webkit-block-placeholder"></div><div>should be rejected earlier. Adding an assert would make sense.</div><div><br><blockquote type="cite">Lex/Preprocessor.cpp:<br>2222<br>2253<br>2329</blockquote><div><br class="webkit-block-placeholder"></div>The calls to <span class="Apple-style-span" style="color: rgb(38, 71, 75); font-family: Monaco; font-size: 10px; ">ReadMacroName verify that the name is an identifier.</span></div><div><font class="Apple-style-span" color="#26474B" face="Monaco" size="2"><span class="Apple-style-span" style="font-size: 10px;"><br class="webkit-block-placeholder"></span></font></div><div><blockquote type="cite">Parse/ParseDecl.cpp:<br>101</blockquote><div><br class="webkit-block-placeholder"></div><div>I'm not sure about this. That call is only reachable if "<span class="Apple-style-span" style="color: rgb(38, 71, 75); font-family: Monaco; font-size: 10px; "><span style="color: #3f6e74">Tok</span><span style="color: #000000">.</span><span style="color: #2e0d6e">is</span><span style="color: #000000">(</span><span style="color: #3f6e74">tok</span><span style="color: #000000">::</span>identifier<span style="color: #000000">) || </span>isDeclarationSpecifier<span style="color: #000000">()". It is unclear to me that all declspecs have identifiers. Steve?</span></span></div><br><blockquote type="cite">1467</blockquote><div><br class="webkit-block-placeholder"></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; color: rgb(196, 26, 22); "><span style="color: #000000"> </span><span style="color: #643820">assert</span><span style="color: #000000">(</span><span style="color: #3f6e74">Tok</span><span style="color: #000000">.</span><span style="color: #2e0d6e">is</span><span style="color: #000000">(</span><span style="color: #3f6e74">tok</span><span style="color: #000000">::</span><span style="color: #26474b">kw_typeof</span><span style="color: #000000">) && </span>"Not a typeof specifier"<span style="color: #000000">);</span></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "> <span style="color: #aa0d91">const</span> <span style="color: #3f6e74">IdentifierInfo</span> *BuiltinII = <span style="color: #3f6e74">Tok</span>.<span style="color: #26474b">getIdentifierInfo</span>();</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "><br class="webkit-block-placeholder"></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; ">The assertion verifies that the token is a keyword, which has an identifier ptr. This code is trying to preserve __typeof__ vs typeof in a diagnostic.</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "><br></div><blockquote type="cite">Parse/ParseExpr.cpp:<br>216</blockquote><div><br class="webkit-block-placeholder"></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; ">ParseExpressionWithLeadingIdentifier is only called with an identifier as IdTok.</div><blockquote type="cite"><br>247</blockquote><div><br class="webkit-block-placeholder"></div>likewise for <span class="Apple-style-span" style="font-family: Monaco; font-size: 10px; ">ParseAssignmentExprWithLeadingIdentifier.</span></div><div><font class="Apple-style-span" face="Monaco" size="2"><span class="Apple-style-span" style="font-size: 10px;"><br></span></font><blockquote type="cite">(785)</blockquote><div><br class="webkit-block-placeholder"></div><div>This is only called with these 4 keywords as the current token:</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; color: rgb(38, 71, 75); "><span style="color: #000000"> </span><span style="color: #aa0d91">case</span><span style="color: #000000"> </span><span style="color: #3f6e74">tok</span><span style="color: #000000">::</span>kw___builtin_va_arg<span style="color: #000000">:</span></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; color: rgb(38, 71, 75); "><span style="color: #000000"> </span><span style="color: #aa0d91">case</span><span style="color: #000000"> </span><span style="color: #3f6e74">tok</span><span style="color: #000000">::</span>kw___builtin_offsetof<span style="color: #000000">:</span></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; color: rgb(38, 71, 75); "><span style="color: #000000"> </span><span style="color: #aa0d91">case</span><span style="color: #000000"> </span><span style="color: #3f6e74">tok</span><span style="color: #000000">::</span>kw___builtin_choose_expr<span style="color: #000000">:</span></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; color: rgb(38, 71, 75); "><span style="color: #000000"> </span><span style="color: #aa0d91">case</span><span style="color: #000000"> </span><span style="color: #3f6e74">tok</span><span style="color: #000000">::</span>kw___builtin_types_compatible_p<span style="color: #000000">:</span></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; color: rgb(38, 71, 75); "><font class="Apple-style-span" color="#000000"><br class="webkit-block-placeholder"></font></div><blockquote type="cite">Parse/Parser.cpp:<br>377 (one of the bugs, fixed with the patch</blockquote><div><br class="webkit-block-placeholder"></div>Ok.</div><div><br><blockquote type="cite"><br>Parse/ParseObjc.cpp:<br>304<br>325 (but only because of strange identation because of tabs instead of spaces -- fixed in the attached patch as well)<br>(476)<br>1130 (one of the bugs, fixed with the patch)<br>1164 (one of the bugs, fixed with the patch)<br>1235<br><br>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</blockquote><div><br class="webkit-block-placeholder"></div><div>This would also require the Token class to be polymorphic, which is a non-starter. Another potential solution would be to make getIdentifierInfo() always assert that the pointer is non-null. This would require callers to call Tok.hasIdentifierInfo() if they don't know it is valid or to add a getIdentifierInfoOrNull() method.</div><br><blockquote type="cite">An unrelated crash that I found on the way is:<br><br> int main()<br> {<br> id a;<br> [a bla:0 6:7];<br> }<br><br>(crashes somewhere in sema, something like this should be put in test/Parse/objc-messaging-1.m)<br><br><br>And here's an inconsistency with gcc:<br><br> int @interface bla ; // ?? this is valid objc?<br> @end<br><br>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.</blockquote><div><br class="webkit-block-placeholder"></div><div>I'll let Steve and Fariborz chime in on these.</div><div><br class="webkit-block-placeholder"></div><blockquote type="cite">ps: I also converted a few tabs to spaces<br></blockquote></div><br><div>Thanks! It would make it easier to review the patch if you kept the mechanical pieces separate from the changes that require review, but I appreciate the patch.</div><div><br class="webkit-block-placeholder"></div><div>As an aside, things will probably pick up in early january, many people are out for the holidays.</div><div><br class="webkit-block-placeholder"></div><div>-Chris</div></body></html>