[modules] No longer include stdlib.h from mm_malloc.h.

[teemperor]

Summary

The GNU C library includes headers from the _Builtin_intrinsics module. As the _Builtin_intrinsics module via
the mm_malloc.h header also includes the stdlib.h header from libc, we get a cyclic dependency with
-fmodules enabled. The best way to solve this is seems to be removing the stdlib.h include from mm_malloc.h
and make the redeclarations in there work without the include.

This patch is doing this in two steps:

1. It reverts some of the changes done in r119958 which re-added the include to mm_malloc.h and removed the forward declarations.

2. It expands the workaround in Sema::CheckEquivalentExceptionSpec to also work in the case where we first declare a function with a missing empty exception specification and then redeclare it with an empty exception specification.

The second part is necessary because the current workaround only works in the case where the redeclaration
is missing an empty exception specification and the #include <stdlib.h> before our redeclaration ensured that
we always have our declarations in this expected order.

I compiled a few projects with this patch (curl, ncnn, opencv, openjpeg, scummvm, sqlite, zlib), and it doesn't
seem to break any compilation there.

[bruno]

test/CXX/except/except.spec/libc-empty-except.cpp

4

Since this is intended to fix a problem you are seeing with modules on, perhaps you should create modulemaps and test this with -fmodules?

7

In a testcase like this but using the actual real headers, if you do:

#include <xmmintrin.h>
#include <stdlib.h>

does it also compiles fine?

[chandlerc]

I think this is more a question for Richard... Add me back to the reviewers if there is a specific need for my input here.

[teemperor]

• Added more test cases for reverse include order and clang modules.

[v.g.vassilev]

test/CXX/except/except.spec/libc-empty-except.cpp

28

Maybe we should reverse the includes here as we discussed offline.

[rsmith]

lib/Sema/SemaExceptionSpec.cpp

353

Updating the old function's type here can potentially violate our AST invariants, especially in C++17 where it will change the canonical type of the function. You would need to at least notify AST mutation listeners, and iterate over all previous declarations rather than only updating one of them. (You're also throwing away things like calling conventions here.)

Sema::UpdateExceptionSpec handles these details for you.

[teemperor]

(Just marking this as "Plan changes" because otherwise it's just stuck in my "Waiting on review" queue).

[teemperor]

• Now using UpdateExceptionSpec.
• Added a comment to the SemaExceptionSpec.cpp code why we are permitting this.

[teemperor]

test/CXX/except/except.spec/libc-empty-except.cpp

7

(I already answered this offline back then, but for the record again): Yes, we tried this actual code and I think the test case (with the actual reverse fix applied) also should cover this case.

28

Thanks!

[teemperor]

• Reverted unintended change to test case that slipped into the latest diff.

[rsmith]

lib/Headers/mm_malloc.h

42

Add a space before (free), please. (Here and above.)

test/CXX/except/except.spec/Inputs/clang/mm_malloc.h

3

Do you mean this might be a redeclaration of a glibc function? Presumably this file is supposed to correspond to our builtin header, not a glibc one.

[joerg]

Please check the history of the file for some of the problems with the redefinition. I'm quite against this change.

[teemperor]

• Removed comment about redeclaring free in the test. That's wasn't correctly formulated and is anyway no longer true now that the test case including this file got bigger.
• Fixed formatting in the free declaration.

(Thanks, Richard).

[v.g.vassilev]

ping...

[teemperor]

@joerg Yeah, we saw the commit explaining why the original fwd declaration patch was reverted. However, from what I can see, we only have three ways to fix the cyclic dependency between glibc and Clang's internal module:

1. We say that we don't support including mm_malloc.h from the module containing stdlib.h.
2. We don't include mm_malloc.h (and the internal headers that include mm_malloc.h) in Clang's internal module.
3. We add this workaround to allow us to forward declare free/malloc here.

Solution 1 essentially means that we won't support a glibc with modules until they work around this issue. And solution 2 means that all affected headers in clang's internal module have to be textually copied into modules that include them.

So after some offline discussion on how to proceed with this, the plan we came up with was to land this patch (which implements solution 3) and then see how/if we can work around the problems that users experience because of it (otherwise we would revert).

I don't have a strict preference between solution 2 and 3 at this point. So if @joerg thinks that this approach is not the right one, then I would just make a new patch that removes the affected headers from Clang's internal module.

[joerg]

This header is used on systems without glibc. So please don't argue about behavior based only on that. Granted, most other libc implementation are less annoying when it comes to free and malloc, but still.

[teemperor]

So, the idea of going back to the headers and see if we can potentially remove mm_malloc from the modulemap didn't work out (mostly because a lot of headers include it indirectly).

However, when going back to the original issue i noticed that the stdlib.h that's at fault is the one from stdlibc++, not the glibc one. In fact, the issue becomes more clear when re-adding cstdlib to my stl modulemap:

. /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/8.2.1/../../../../include/c++/8.2.1/stdlib.h
While building module 'stl11' imported from /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/8.2.1/../../../../include/c++/8.2.1/stdlib.h:36:
While building module '_Builtin_intrinsics' imported from /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/8.2.1/../../../../include/c++/8.2.1/x86_64-pc-linux-gnu/bits/opt_random.h:34:
In file included from <module-includes>:2:
In file included from /usr/lib/clang/7.0.1/include/immintrin.h:32:
In file included from /usr/lib/clang/7.0.1/include/xmmintrin.h:39:
In file included from /usr/lib/clang/7.0.1/include/mm_malloc.h:27:
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/8.2.1/../../../../include/c++/8.2.1/stdlib.h:36:11: fatal error: cyclic dependency in module 'stl11': stl11 -> _Builtin_intrinsics
      -> stl11
# include <cstdlib>
          ^
While building module 'stl11' imported from /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/8.2.1/../../../../include/c++/8.2.1/stdlib.h:36:
In file included from <module-includes>:52:
In file included from /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/8.2.1/../../../../include/c++/8.2.1/random:50:
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/8.2.1/../../../../include/c++/8.2.1/x86_64-pc-linux-gnu/bits/opt_random.h:34:10: fatal error: could not build module
      '_Builtin_intrinsics'
#include <pmmintrin.h>
 ~~~~~~~~^
In file included from test.cpp:2:
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/8.2.1/../../../../include/c++/8.2.1/stdlib.h:36:11: fatal error: could not build module 'stl11'
# include <cstdlib>
  ~~~~~~~~^
3 errors generated.

So to summarize: including stdlib.h (or cstdlib) is triggering our stl module to build, which in turn causes our random module to trigger which includes mm_malloc indirectly. If we move cstdlib and stdlib.h in their own modules we break the cyclic dependency. It's not optimal but I think it's less hacky than this patch or any of the other workarounds we came up with so far.

lib/Headers/mm_malloc.h

  * THE SOFTWARE.
  *
  *===-----------------------------------------------------------------------===
  */
 
 #ifndef __MM_MALLOC_H
 #define __MM_MALLOC_H
 
-#include <stdlib.h>
+#include <stddef.h>
 
 #ifdef _WIN32
 #include <malloc.h>
 #else
 #ifndef __cplusplus
 extern int posix_memalign(void **__memptr, size_t __alignment, size_t __size);
+extern void (free)(void *ptr);
+extern void *(malloc)(size_t size) __attribute__((__malloc__));
 #else
-// Some systems (e.g. those with GNU libc) declare posix_memalign with an
-// exception specifier. Via an "egregious workaround" in
-// Sema::CheckEquivalentExceptionSpec, Clang accepts the following as a valid
-// redeclaration of glibc's declaration.
+// Some systems (e.g. those with GNU libc) declare some of these functions with
+// an exception specifier. Via an "egregious workaround" in
+// Sema::CheckEquivalentExceptionSpec, Clang accepts the following as valid
+// (re)declarations of glibc's declarations.
 extern "C" int posix_memalign(void **__memptr, size_t __alignment, size_t __size);
+extern "C" void (free)(void *ptr);
+extern "C" void *(malloc)(size_t size) __attribute__((__malloc__));
 #endif
 #endif
 
 #if !(defined(_WIN32) && defined(_mm_malloc))
 static __inline__ void *__attribute__((__always_inline__, __nodebug__,
                                        __malloc__))
 _mm_malloc(size_t __size, size_t __align)
 {

lib/Sema/SemaExceptionSpec.cpp

 }
 
 static bool CheckEquivalentExceptionSpecImpl(
     Sema &S, const PartialDiagnostic &DiagID, const PartialDiagnostic &NoteID,
     const FunctionProtoType *Old, SourceLocation OldLoc,
     const FunctionProtoType *New, SourceLocation NewLoc,
     bool *MissingExceptionSpecification = nullptr,
     bool *MissingEmptyExceptionSpecification = nullptr,
+    bool *ExtraEmptyExceptionSpecification = nullptr,
     bool AllowNoexceptAllMatchWithNoSpec = false, bool IsOperatorNew = false);
 
 /// Determine whether a function has an implicitly-generated exception
 /// specification.
 static bool hasImplicitExceptionSpec(FunctionDecl *Decl) {
   if (!isa<CXXDestructorDecl>(Decl) &&
       Decl->getDeclName().getCXXOverloadedOperator() != OO_Delete &&
       Decl->getDeclName().getCXXOverloadedOperator() != OO_Array_Delete)
     return false;
 
   // For a function that the user didn't declare:
   //  - if this is a destructor, its exception specification is implicit.
   //  - if this is 'operator delete' or 'operator delete[]', the exception
   //    specification is as-if an explicit exception specification was given
   //    (per [basic.stc.dynamic]p2).
   if (!Decl->getTypeSourceInfo())
     return isa<CXXDestructorDecl>(Decl);
 
   const FunctionProtoType *Ty =
     Decl->getTypeSourceInfo()->getType()->getAs<FunctionProtoType>();
   return !Ty->hasExceptionSpec();
 }
 
+/// Returns true if the given function is a function/builtin with C linkage
+/// and from a system header.
+static bool isCSystemFuncOrBuiltin(FunctionDecl *D, ASTContext &Context) {
+  return (D->getLocation().isInvalid() ||
+          Context.getSourceManager().isInSystemHeader(D->getLocation()) ||
+          D->getBuiltinID()) &&
+         D->isExternC();
+}
+
 bool Sema::CheckEquivalentExceptionSpec(FunctionDecl *Old, FunctionDecl *New) {
   // Just completely ignore this under -fno-exceptions prior to C++17.
   // In C++17 onwards, the exception specification is part of the type and
   // we will diagnose mismatches anyway, so it's better to check for them here.
   if (!getLangOpts().CXXExceptions && !getLangOpts().CPlusPlus17)
     return false;
 
   OverloadedOperatorKind OO = New->getDeclName().getCXXOverloadedOperator();
   bool IsOperatorNew = OO == OO_New || OO == OO_Array_New;
   bool MissingExceptionSpecification = false;
   bool MissingEmptyExceptionSpecification = false;
+  bool ExtraEmptyExceptionSpecification = false;
+  bool *AllowExtraEmptyExceptionSpecification = nullptr;
+
+  // If both functions are from C functions from system headers, we want to
+  // know if the redeclaration has an additional empty exception specification.
+  if (isCSystemFuncOrBuiltin(Old, Context) &&
+      isCSystemFuncOrBuiltin(New, Context))
+    AllowExtraEmptyExceptionSpecification = &ExtraEmptyExceptionSpecification;
 
   unsigned DiagID = diag::err_mismatched_exception_spec;
   bool ReturnValueOnError = true;
   if (getLangOpts().MicrosoftExt) {
     DiagID = diag::ext_mismatched_exception_spec;
     ReturnValueOnError = false;
   }
 
   // If we're befriending a member function of a class that's currently being
   // defined, we might not be able to work out its exception specification yet.
   // If not, defer the check until later.
   if (exceptionSpecNotKnownYet(Old) || exceptionSpecNotKnownYet(New)) {
     DelayedEquivalentExceptionSpecChecks.push_back({New, Old});
     return false;
   }
 
   // Check the types as written: they must match before any exception
   // specification adjustment is applied.
   if (!CheckEquivalentExceptionSpecImpl(
           *this, PDiag(DiagID), PDiag(diag::note_previous_declaration),
           Old->getType()->getAs<FunctionProtoType>(), Old->getLocation(),
           New->getType()->getAs<FunctionProtoType>(), New->getLocation(),
           &MissingExceptionSpecification, &MissingEmptyExceptionSpecification,
+          AllowExtraEmptyExceptionSpecification,
           /*AllowNoexceptAllMatchWithNoSpec=*/true, IsOperatorNew)) {
     // C++11 [except.spec]p4 [DR1492]:
     //   If a declaration of a function has an implicit
     //   exception-specification, other declarations of the function shall
     //   not specify an exception-specification.
     if (getLangOpts().CPlusPlus11 && getLangOpts().CXXExceptions &&
         hasImplicitExceptionSpec(Old) != hasImplicitExceptionSpec(New)) {
       Diag(New->getLocation(), diag::ext_implicit_exception_spec_mismatch)
         << hasImplicitExceptionSpec(Old);
       if (Old->getLocation().isValid())
         Diag(Old->getLocation(), diag::note_previous_declaration);
     }
     return false;
   }
 
+  const FunctionProtoType *NewProto =
+      New->getType()->castAs<FunctionProtoType>();
+  const FunctionProtoType *OldProto =
+      Old->getType()->castAs<FunctionProtoType>();
+
+  // We know that both decls have C linkage, are from a system header and that
+  // the "new" decl has an extra empty exception specification "throw()". In
+  // this case we don't emit an error but instead update the previous
+  // declaration with the exception specification from the "new" decl. This is
+  // usually not permitted, but it's a necessary hack required for the
+  // redeclarations of free/malloc in mm_malloc.h.
+  if (ExtraEmptyExceptionSpecification && OldProto && NewProto) {
+    UpdateExceptionSpec(Old, NewProto->getExtProtoInfo().ExceptionSpec);
+    return false;
+  }
+
   // The failure was something other than an missing exception
   // specification; return an error, except in MS mode where this is a warning.
   if (!MissingExceptionSpecification)
     return ReturnValueOnError;
 
-  const FunctionProtoType *NewProto =
-    New->getType()->castAs<FunctionProtoType>();
-
   // The new function declaration is only missing an empty exception
   // specification "throw()". If the throw() specification came from a
   // function in a system header that has C linkage, just add an empty
   // exception specification to the "new" declaration. Note that C library
   // implementations are permitted to add these nothrow exception
   // specifications.
   //
   // Likewise if the old function is a builtin.
   if (MissingEmptyExceptionSpecification && NewProto &&
-      (Old->getLocation().isInvalid() ||
-       Context.getSourceManager().isInSystemHeader(Old->getLocation()) ||
-       Old->getBuiltinID()) &&
-      Old->isExternC()) {
+      isCSystemFuncOrBuiltin(Old, Context)) {
     New->setType(Context.getFunctionType(
         NewProto->getReturnType(), NewProto->getParamTypes(),
         NewProto->getExtProtoInfo().withExceptionSpec(EST_DynamicNone)));
     return false;
   }
 
-  const FunctionProtoType *OldProto =
-    Old->getType()->castAs<FunctionProtoType>();
-
   FunctionProtoType::ExceptionSpecInfo ESI = OldProto->getExceptionSpecType();
   if (ESI.Type == EST_Dynamic) {
     // FIXME: What if the exceptions are described in terms of the old
     // prototype's parameters?
     ESI.Exceptions = OldProto->exceptions();
   }
 
   if (ESI.Type == EST_NoexceptFalse)

 /// a problem. If \c true is returned, either a diagnostic has already been
 /// produced or \c *MissingExceptionSpecification is set to \c true.
 static bool CheckEquivalentExceptionSpecImpl(
     Sema &S, const PartialDiagnostic &DiagID, const PartialDiagnostic &NoteID,
     const FunctionProtoType *Old, SourceLocation OldLoc,
     const FunctionProtoType *New, SourceLocation NewLoc,
     bool *MissingExceptionSpecification,
     bool *MissingEmptyExceptionSpecification,
+    bool *ExtraEmptyExceptionSpecification,
     bool AllowNoexceptAllMatchWithNoSpec, bool IsOperatorNew) {
   if (MissingExceptionSpecification)
     *MissingExceptionSpecification = false;
 
   if (MissingEmptyExceptionSpecification)
     *MissingEmptyExceptionSpecification = false;
 
+  if (ExtraEmptyExceptionSpecification)
+    *ExtraEmptyExceptionSpecification = false;
+
   Old = S.ResolveExceptionSpec(NewLoc, Old);
   if (!Old)
     return false;
   New = S.ResolveExceptionSpec(NewLoc, New);
   if (!New)
     return false;
 
   // C++0x [except.spec]p3: Two exception-specifications are compatible if:
if (WithExceptions && WithExceptions->getNumExceptions() == 1) {
           if (ExRecord->isInStdNamespace()) {
             return false;
           }
         }
       }
     }
   }
 
+  if (ExtraEmptyExceptionSpecification && !Old->hasExceptionSpec() &&
+      New->hasExceptionSpec()) {
+    *ExtraEmptyExceptionSpecification = true;
+    return true;
+  }
+
   // If the caller wants to handle the case that the new function is
   // incompatible due to a missing exception specification, let it.
   if (MissingExceptionSpecification && OldEST != EST_None &&
       NewEST == EST_None) {
     // The old type has an exception specification of some sort, but
     // the new type does not.
     *MissingExceptionSpecification = true;
 

test/CXX/except/except.spec/Inputs/clang/mm_malloc.h

+// Missing throw() is allowed in this case as we are in a system header.
+extern "C" void free(void *ptr);

test/CXX/except/except.spec/Inputs/clang/module.modulemap

+module builtin [system] {
+  header "mm_malloc.h"
+  export *
+}

test/CXX/except/except.spec/Inputs/libc/module.modulemap

+module libc [system] {
+  header "stdlib.h"
+  export *
+}

test/CXX/except/except.spec/Inputs/libc/stdlib.h

+// Declare 'free' like glibc with a empty exception specifier.
+extern "C" void free(void *ptr) throw();

test/CXX/except/except.spec/libc-empty-except.cpp

+// RUN: %clang_cc1 -std=c++11 -isystem %S/Inputs/clang -internal-externc-isystem %S/Inputs/libc -fexceptions -fcxx-exceptions -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c++11 -isystem %S/Inputs/clang -internal-externc-isystem %S/Inputs/libc -fexceptions -fcxx-exceptions -fsyntax-only -verify -DREVERSE %s
+
+// One of the headers is in a user include, so our redeclaration should fail.
+// RUN: not %clang_cc1 -std=c++11 -I %S/Inputs/clang -internal-externc-isystem %S/Inputs/libc -fexceptions -fcxx-exceptions -fsyntax-only -verify %s
+// RUN: not %clang_cc1 -std=c++11 -isystem %S/Inputs/clang -I %S/Inputs/libc -fexceptions -fcxx-exceptions -fsyntax-only -verify -DREVERSE %s
+
+// The same test cases again with enabled modules.
+// The modules cases *all* pass because we marked both as [system].
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visibility -fimplicit-module-maps -fmodules-cache-path=%t \
+// RUN:            -std=c++11 -isystem %S/Inputs/clang -internal-externc-isystem %S/Inputs/libc -fexceptions -fcxx-exceptions -fsyntax-only -verify %s
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visibility -fimplicit-module-maps -fmodules-cache-path=%t \
+// RUN:            -std=c++11 -isystem %S/Inputs/clang -internal-externc-isystem %S/Inputs/libc -fexceptions -fcxx-exceptions -fsyntax-only -verify -DREVERSE %s
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visibility -fimplicit-module-maps -fmodules-cache-path=%t \
+// RUN:            -std=c++11 -I %S/Inputs/clang -internal-externc-isystem %S/Inputs/libc -fexceptions -fcxx-exceptions -fsyntax-only -verify %s
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visibility -fimplicit-module-maps -fmodules-cache-path=%t \
+// RUN:            -std=c++11 -isystem %S/Inputs/clang -I %S/Inputs/libc -fexceptions -fcxx-exceptions -fsyntax-only -verify -DREVERSE %s
+
+// expected-no-diagnostics
+#ifdef REVERSE
+#include "mm_malloc.h"
+#include "stdlib.h"
+#else
+#include "mm_malloc.h"
+#include "stdlib.h"
+#endif
+
+void f() {
+  free(nullptr);
+}