Teemperor / clang-modules-bugs

2 stars 0 forks source link

Conflicting declarations of with posix_memalign with glibc #3

Open Teemperor opened 7 years ago

Teemperor commented 7 years ago

So when modularizing libc, I encountered this problem:

While building module 'eigen3' imported from /home/travis/build/Teemperor/ClangModulesCMake/build/eigen3/main.cpp:2:
In file included from <module-includes>:1:
In file included from /usr/include/eigen3/Eigen/Cholesky:4:
In file included from /usr/include/eigen3/Eigen/Core:101:
In file included from /usr/include/clang/4.0.1/include/emmintrin.h:27:
In file included from /usr/include/clang/4.0.1/include/xmmintrin.h:39:
/usr/include/clang/4.0.1/include/mm_malloc.h:39:16: error: 'posix_memalign' is missing exception specification 'throw()'
extern "C" int posix_memalign(void **__memptr, size_t __alignment, size_t __size);
               ^
/usr/include/stdlib.h:503:12: note: previous declaration is here
extern int posix_memalign (void **__memptr, size_t __alignment, size_t __size)
           ^

Example error in travis

Looking at the mm_malloc.h, it seems this error is known and just needs to be properly fixed in modules:

34 #else
35 // Some systems (e.g. those with GNU libc) declare posix_memalign with an
36 // exception specifier. Via an "egregious workaround" in
37 // Sema::CheckEquivalentExceptionSpec, Clang accepts the following as a valid
38 // redeclaration of glibc's declaration.
39 extern "C" int posix_memalign(void **__memptr, size_t __alignment, size_t __size);
40 #endif
Teemperor commented 7 years ago

See also this PR: https://github.com/Teemperor/ClangModulesCMake/pull/44

Teemperor commented 7 years ago

So, the "workaround" seems to be no longer around? The comment from the header is from 2010 and the workaround back then was this:

  // 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. This is an
  // egregious workaround for glibc, which adds throw() specifications
  // to many libc functions as an optimization. Unfortunately, that
  // optimization isn't permitted by the C++ standard, so we're forced
  // to work around it here.
  if (MissingEmptyExceptionSpecification &&
      isa<FunctionProtoType>(New->getType()) &&
      (Old->getLocation().isInvalid() ||
       Context.getSourceManager().isInSystemHeader(Old->getLocation())) &&
      Old->isExternC()) {
    const FunctionProtoType *NewProto
      = cast<FunctionProtoType>(New->getType());
    QualType NewType = Context.getFunctionType(NewProto->getResultType(),
                                               NewProto->arg_type_begin(),
                                               NewProto->getNumArgs(),
                                               NewProto->isVariadic(),
                                               NewProto->getTypeQuals(),
                                               true, false, 0, 0,
                                               NewProto->getExtInfo());
    New->setType(NewType);
    return false;
  }
Teemperor commented 7 years ago

Ah, seems Richard has just adapted this workaround in 2016:

  // 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()) {
    New->setType(Context.getFunctionType(
        NewProto->getReturnType(), NewProto->getParamTypes(),
        NewProto->getExtProtoInfo().withExceptionSpec(EST_DynamicNone)));
    return false;
  }
Teemperor commented 7 years ago

Added a reproducer to the repo: https://github.com/Teemperor/clang-modules-bugs/tree/master/posix_memalign_repro

Teemperor commented 7 years ago

Our modulemap was not marked as system, so event though it was a in a system include it got built as a non-system and then above's check obviously fails.