r240571 - [Preprocessor] Iterating over all macros should include those from modules.

[Richard Smith]

On Thu, Jun 25, 2015 at 12:53 PM, Jordan Rose <jordan_rose at apple.com> wrote:

>
> On Jun 25, 2015, at 11:54, Justin Bogner <mail at justinbogner.com> wrote:
>
> Jordan Rose <jordan_rose at apple.com> writes:
>
> Author: jrose
> Date: Wed Jun 24 14:27:02 2015
> New Revision: 240571
>
> URL: http://llvm.org/viewvc/llvm-project?rev=240571&view=rev
> Log:
> [Preprocessor] Iterating over all macros should include those from modules.
>
> So, iterate over the list of macros mentioned in modules, and make sure
> those
> are in the master table.
>
>
> This introduces a null dereference in Preprocessor::findDirectiveAtLoc.
> It triggers in clang/test/Modules/crashes.m, so you can find it by
> adding an assert next to the very relevant sounding FIXME like so:
>
> --- a/include/clang/Lex/Preprocessor.h
> +++ b/include/clang/Lex/Preprocessor.h
> @@ -454,6 +454,7 @@ class Preprocessor : public
> RefCountedBase<Preprocessor> {
>     MacroDirective::DefInfo findDirectiveAtLoc(SourceLocation Loc,
>                                                SourceManager &SourceMgr)
> const {
>       // FIXME: Incorporate module macros into the result of this.
> +      assert(getLatest() != nullptr);
>       return getLatest()->findDirectiveAtLoc(Loc, SourceMgr);
>     }
>
> I found this with ubsan, but I still need a couple of clang patches
> reviewed before I can make a bot to catch such things. For anyone
> interested:
>
>
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20150622/131431.html
>
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20150622/131432.html
>
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20150622/131433.html
>
>
> Hm, thanks. I'm not sure whether this means we/I should be more
> circumspect about what gets added to the Macros table, or whether
> findDirectiveAtLoc should not assume that everything in the Macros table is
> currently valid.
>

I think this is a bug in findDirectiveAtLoc(): it's assuming that every
MacroState has at least one local MacroDirective, which is not necessarily
the case. I think we can just return a default-constructed DefInfo here if
there is no latest macro directive, at least for now. (Longer-term, it'd be
nice if providing a location within a module would get the latest macro
directive visible at that location within that module, but it doesn't seem
like a particularly high priority.)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150625/b049c1c0/attachment.html>