Page MenuHomePhabricator

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

Authored by teemperor on Feb 28 2018, 6:25 AM.

Details

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.
  1. 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.

Diff Detail

Event Timeline

teemperor created this revision.Feb 28 2018, 6:25 AM
bruno added a subscriber: bruno.Mar 23 2018, 5:00 PM
bruno added inline comments.
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 resigned from this revision.Mar 26 2018, 1:06 PM

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.

  • Added more test cases for reverse include order and clang modules.
v.g.vassilev added inline comments.Apr 11 2018, 12:35 AM
test/CXX/except/except.spec/libc-empty-except.cpp
28

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

rsmith added inline comments.Apr 11 2018, 2:23 AM
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 planned changes to this revision.Jul 27 2018, 10:59 AM
teemperor marked an inline comment as done.

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

  • Now using UpdateExceptionSpec.
  • Added a comment to the SemaExceptionSpec.cpp code why we are permitting this.
teemperor marked 3 inline comments as done.Aug 28 2018, 10:54 AM
teemperor added inline comments.
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 updated this revision to Diff 162940.Aug 28 2018, 1:13 PM
teemperor marked an inline comment as done.
  • Reverted unintended change to test case that slipped into the latest diff.
rsmith accepted this revision.Aug 28 2018, 3:51 PM
rsmith added inline comments.
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.

This revision is now accepted and ready to land.Aug 28 2018, 3:51 PM
joerg added a subscriber: joerg.Sep 4 2018, 4:41 AM

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

teemperor updated this revision to Diff 164885.Sep 11 2018, 8:10 AM
teemperor marked 3 inline comments as done.
  • 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).

@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 added a comment.Dec 11 2018, 1:15 PM

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 abandoned this revision.Jan 22 2019, 1:37 PM

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.