r217275 - Move the initialization of VAListTagName after InitializeSema()

[David Blaikie]

On Fri, Sep 5, 2014 at 1:24 PM, Ben Langmuir <blangmuir at apple.com> wrote:

> Author: benlangmuir
> Date: Fri Sep  5 15:24:27 2014
> New Revision: 217275
>
> URL: http://llvm.org/viewvc/llvm-project?rev=217275&view=rev
> Log:
> Move the initialization of VAListTagName after InitializeSema()
>
> This innocuous statement to get the identifier info for __va_list_tag
> was causing an assertion failure:
>   NextIsPrevious() && "decl became non-canonical unexpectedly"
> if the __va_list_tag identifier was found in a PCH in some
> circumstances, because it was looked up before the ASTReader had a Sema
> object to use to find existing decls to merge with.
>
> We could possibly move getting the identifier info even later, or make
> it lazy if we wanted to, but this seemed like the minimal change.
>
> Now why a PCH would have this identifier in the first place is a bit
> mysterious. This seems to be related to the global module index in some
> way, because when the test case is built without the global module index
> it will not emit an identifier for __va_list_tag into the PCH, but with
> the global module index it does.
>
> Added:
>     cfe/trunk/test/Modules/Inputs/va_list/
>     cfe/trunk/test/Modules/Inputs/va_list/module.modulemap
>     cfe/trunk/test/Modules/Inputs/va_list/va_list_a.h
>     cfe/trunk/test/Modules/Inputs/va_list/va_list_b.h
>     cfe/trunk/test/Modules/va_list.m
> Modified:
>     cfe/trunk/lib/Sema/Sema.cpp
>
> Modified: cfe/trunk/lib/Sema/Sema.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/Sema.cpp?rev=217275&r1=217274&r2=217275&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Sema/Sema.cpp (original)
> +++ cfe/trunk/lib/Sema/Sema.cpp Fri Sep  5 15:24:27 2014
> @@ -69,8 +69,6 @@ PrintingPolicy Sema::getPrintingPolicy(c
>  void Sema::ActOnTranslationUnitScope(Scope *S) {
>    TUScope = S;
>    PushDeclContext(S, Context.getTranslationUnitDecl());
> -
> -  VAListTagName = PP.getIdentifierInfo("__va_list_tag");
>  }
>
>  Sema::Sema(Preprocessor &pp, ASTContext &ctxt, ASTConsumer &consumer,
> @@ -155,6 +153,10 @@ void Sema::Initialize() {
>        = dyn_cast_or_null<ExternalSemaSource>(Context.getExternalSource()))
>      ExternalSema->InitializeSema(*this);
>
> +  // This needs to happen after ExternalSemaSource::InitializeSema(this)
> or we
> +  // will not be able to merge any duplicate __va_list_tag decls
> correctly.
> +  VAListTagName = PP.getIdentifierInfo("__va_list_tag");
> +
>    // Initialize predefined 128-bit integer types, if needed.
>    if (Context.getTargetInfo().hasInt128Type()) {
>      // If either of the 128-bit integer types are unavailable to name
> lookup,
>
> Added: cfe/trunk/test/Modules/Inputs/va_list/module.modulemap
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/va_list/module.modulemap?rev=217275&view=auto
>
> ==============================================================================
> --- cfe/trunk/test/Modules/Inputs/va_list/module.modulemap (added)
> +++ cfe/trunk/test/Modules/Inputs/va_list/module.modulemap Fri Sep  5
> 15:24:27 2014
> @@ -0,0 +1,5 @@
> +module stdarg [system] {
> +  header "stdarg.h" // note: supplied by the compiler
>

I've simplified this test in 217290 so it doesn't depend on the presence of
system (or compiler) headers. It looks like it didn't need it (I guess objc
provides va_list by default?). Let me know if there's more to it than that,
but I did verify that the test still (with my changes) fails without your
patch.


> +}
> +module va_list_a { header "va_list_a.h" }
> +module va_list_b { header "va_list_b.h" }
>
> Added: cfe/trunk/test/Modules/Inputs/va_list/va_list_a.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/va_list/va_list_a.h?rev=217275&view=auto
>
> ==============================================================================
> --- cfe/trunk/test/Modules/Inputs/va_list/va_list_a.h (added)
> +++ cfe/trunk/test/Modules/Inputs/va_list/va_list_a.h Fri Sep  5 15:24:27
> 2014
> @@ -0,0 +1,2 @@
> + at import stdarg;
> +int  vprintf(const char * __restrict, va_list);
>
> Added: cfe/trunk/test/Modules/Inputs/va_list/va_list_b.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/va_list/va_list_b.h?rev=217275&view=auto
>
> ==============================================================================
> --- cfe/trunk/test/Modules/Inputs/va_list/va_list_b.h (added)
> +++ cfe/trunk/test/Modules/Inputs/va_list/va_list_b.h Fri Sep  5 15:24:27
> 2014
> @@ -0,0 +1,2 @@
> + at import va_list_a;
> +void NSLogv(id, va_list);
>
> Added: cfe/trunk/test/Modules/va_list.m
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/va_list.m?rev=217275&view=auto
>
> ==============================================================================
> --- cfe/trunk/test/Modules/va_list.m (added)
> +++ cfe/trunk/test/Modules/va_list.m Fri Sep  5 15:24:27 2014
> @@ -0,0 +1,27 @@
> +// RUN: rm -rf %t
> +// RUN: %clang_cc1 -triple x86_64-apple-macosx10 -fmodules
> -fmodules-cache-path=%t \
> +// RUN:     -fmodules-ignore-macro=PREFIX -DPREFIX -I %S/Inputs/va_list \
> +// RUN:     -x objective-c-header %s -o %t.pch -emit-pch
> +
> +// Include the pch, as a sanity check.
> +// RUN: %clang_cc1 -triple x86_64-apple-macosx10 -fmodules
> -fmodules-cache-path=%t \
> +// RUN:     -fmodules-ignore-macro=PREFIX -I %S/Inputs/va_list
> -include-pch %t.pch \
> +// RUN:     -x objective-c %s -fsyntax-only
> +
> +// Repeat the previous emit-pch, but not we will have a global module
> index.
> +// For some reason, this results in an identifier for __va_list_tag being
> +// emitted into the pch.
> +// RUN: %clang_cc1 -triple x86_64-apple-macosx10 -fmodules
> -fmodules-cache-path=%t \
> +// RUN:     -fmodules-ignore-macro=PREFIX -DPREFIX -I %S/Inputs/va_list \
> +// RUN:     -x objective-c-header %s -o %t.pch -emit-pch
> +
> +// Include the pch, which now has __va_list_tag in it, which needs to be
> merged.
> +// RUN: %clang_cc1 -triple x86_64-apple-macosx10 -fmodules
> -fmodules-cache-path=%t \
> +// RUN:     -fmodules-ignore-macro=PREFIX -I %S/Inputs/va_list
> -include-pch %t.pch \
> +// RUN:     -x objective-c %s -fsyntax-only
> +
> +// rdar://18039719
> +
> +#ifdef PREFIX
> + at import va_list_b;
> +#endif
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140905/f6edceed/attachment.html>