Page MenuHomePhabricator

[Clang][Driver] Re-use the calling process instead of creating a new process for the cc1 invocation
ClosedPublic

Authored by aganea on Nov 4 2019, 1:56 PM.

Details

Summary

This patch is an optimization for speed: whenever possible, it will avoid creating a new process for the -cc1 invocation, and instead will call the cc1 tool inside the calling process (clang tool).
On Windows, this has a moderate impact on build times (please see timings in the comments below)

CFG (control flow guard) is disabled on Windows.

This patch also mildly improves build times on a Haswell 6-core Linux PC.

@Meinersbur reported no change in timings on a many-core Linux machine.


If you'd like to try this on your configuration, use the script below to ensure a standardized unit of measure:
(this is only for Windows)

Warning: the script pulls and reverts all local changes!
Please edit the script before running, to provide the right filename for applying the patch.

The script can be used such as:

# run the test (lengthy, hours!)
> powershell .\abba_test.ps1

By default, it'll do a 2-stage build, including cmake'ing with a fixed set of options; followed by AB/BA testing where it'll alternatively rebuild using Clang 10 with and without the patch, for at least 5 hours. If you want to fiddle the number of hours it runs, you can do:

> powershell .\abba_test.ps1 50

Where '50' represents the number of hours it'll run. You can also skip building the two stages, if you already did it, by setting the second parameter:

> powershell .\abba_test.ps1 50 $True

You then end up with something like that:

Total iterations:  6
     |         Min       |        Mean       |       Median      |         Max       |
   A |  00:11:55.8588259 |  00:12:11.5531275 |  00:12:09.5867878 |  00:12:30.2468040 |
   B |  00:09:35.1556958 |  00:09:52.5302980 |  00:09:55.2329476 |  00:10:04.4994028 |
Diff | -00:02:20.7031301 | -00:02:19.0228296 | -00:02:14.3538402 | -00:02:25.7474012 |

If you find this useful, I can maybe convert it to a python script and send a review separately.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I guess Duncan's comments on https://reviews.llvm.org/D70568 were for this patch. I think it would be good to have:

  • A cmake option that controls the default behavior. It should still fork for Darwin by default, the detection should be done through CMake when it's building for a Darwin platform.
  • A driver/cc1 flag that allows the user to override the behavior.

This will allow users to avoid cc1 forking either by building their own Clang, or by passing in a flag with their build options. WDYT?

I guess Duncan's comments on https://reviews.llvm.org/D70568 were for this patch. I think it would be good to have:

That's right, I was confused about which review I was on.

  • A cmake option that controls the default behavior. It should still fork for Darwin by default, the detection should be done through CMake when it's building for a Darwin platform.
  • A driver/cc1 flag that allows the user to override the behavior.
  • I don't think a -cc1 flag is reasonable, because it affects the caller, not -cc1 itself. I would be confused by this I think.
  • Driver flag seems a bit too user-facing, but probably fine.
  • Another (maybe odd) option would be an environment variable, such as CLANG_FORK_CC1=1 or CLANG_FORK_CC1=0, which overrides the default.

This will allow users to avoid cc1 forking either by building their own Clang, or by passing in a flag with their build options. WDYT?

Right. Some way to control at runtime, especially so we can test both ways of launching in check-clang but also probably useful for other scenarios, and a cmake flag to set the default so vendors can choose.

aganea added a comment.EditedDec 5 2019, 11:28 AM

I did this in driver.cpp:

// Whether the cc1 tool should be called inside the current process, or forked
// to a new new process.
bool UseCC1ProcessFork = CLANG_FORK_CC1;

StringRef ForkCC1Str = ::getenv("CLANG_FORK_CC1");
if (!ForkCC1Str.empty()) {
  UseCC1ProcessFork = ForkCC1Str.compare("1") == 0 ||
                      ForkCC1Str.compare_lower("on") == 0 ||
                      ForkCC1Str.compare_lower("true") == 0;
}
if (!UseCC1ProcessFork) {
  // Here we provide a shortcut for calling the -cc1 cmd-line within the
  // same process, instead of starting a new process. This saves a some
  // execution time of Windows, as process creation can be expensive on
  // that platform.
  TheDriver.CC1Main = &ExecuteCC1Tool;
}

With a new option in cmake to control the default behavior:

set(CLANG_FORK_CC1 OFF BOOL
    "Whether clang should use a new process for the CC1 invocation")

if (APPLE)
  set(CLANG_FORK_CC1 1)
endif()

And the corresponding flag in config.h:

/* If we want to fork a new process clang.exe for the CC1 invocation */
#cmakedefine01 CLANG_FORK_CC1

This option needs to be controlled at large (otherwise some TUs might compile with forking "on" and others with forking "off", depending on what happens in the build scripts), so I thought the env variable would make more sense. Do you think a new cmd-line option is worth it, or is the env var enough?

The environment variable should be enough IMHO

rnk added a comment.Dec 5 2019, 1:14 PM

I'd be fine with the cmake var and the env var and no driver flag.

I think clang developers on Darwin probably want the single-process clang -c behavior (that's how I interpreted what Duncan said), even though Apple as a distributor of clang wants the multi-process behavior. I would skip the platform check and in upstream clang and suggest that Apple customize the CMake variable when producing the XCode tools package. But, that's just my opinion, please disagree if you don't like that idea.

Lastly, we could make the environment variable more cross-platform by calling it CLANG_SPAWN_CC1 to be inclusive of OSs where fork doesn't exist. Besides, doesn't clang use posix_spawn on Mac anyway?

aganea updated this revision to Diff 232861.Dec 9 2019, 7:49 AM
aganea marked 4 inline comments as done.

Addressed all comments.
Added CLANG_SPAWN_CC1 cmake flag & environment variable to control whether we want a cc1 process or not. It is disabled by default. I left out the if (APPLE) part in cmake, as suggested by @rnk.

aganea added inline comments.Dec 9 2019, 7:49 AM
clang/tools/driver/driver.cpp
337

Risky in what way? What would you suggest? The contents of argv are exactly the same in both cases: when called from the CC1Command, or explicitly from the command-line.

hans marked an inline comment as done.Dec 11 2019, 6:32 AM

I'm basically happy with this, just some minor comments.

clang/lib/Driver/Job.cpp
376

I think this gets printed by "clang -###" so maybe just print something short here. Perhaps just an "(in-process)" prefix? Then we don't need to change so many test expectations also.

394

Do we need to disable afterwards?

clang/tools/driver/driver.cpp
268

Maybe just do the "!!" thing like for the environment variables above? It's not pretty, but at least that would be consistent, and it avoids the problem of deciding what values mean "on" and what mean "off".

337

You're right, I suppose the code was already doing this.

niosHD added a subscriber: niosHD.Dec 11 2019, 6:37 AM
Trass3r added a subscriber: Trass3r.Jan 8 2020, 5:52 AM
jdoerfert resigned from this revision.Jan 8 2020, 8:23 AM
aganea updated this revision to Diff 237043.Jan 9 2020, 5:54 AM
aganea marked 4 inline comments as done.
aganea added inline comments.
clang/lib/Driver/Job.cpp
394

I moved this line to clang/tools/driver/driver.cpp like discussed previously.

clang/tools/driver/driver.cpp
268

Not if we want to negate the CLANG_SPAWN_CC1 compile-time flag initialized just above. !!::getenv will only say the option is there, it won't say what we should do. My rationale was, if the env variable is there, its value overrides the compile-time flag. Unless we want something along the lines of CLANG_ENABLE_SPAWN_CC1 / CLANG_DISABLE_SPAWN_CC1?

ychen added a subscriber: ychen.Jan 9 2020, 9:37 AM
hans added inline comments.Jan 10 2020, 8:14 AM
clang/tools/driver/driver.cpp
268

Ah, I didn't think about that.

This feels a bit fragile though, for example if the user sets it to "yes" instead of "on", it won't be obvious that it has the opposite effect. What do you think about only supporting 1 and 0, and printing a helpful error for other values?

Also, the comparisons could be done with equals(_lower) instead of the compare methods.

aganea updated this revision to Diff 237465.Jan 10 2020, 5:49 PM
aganea marked 2 inline comments as done.
aganea added inline comments.
clang/tools/driver/driver.cpp
268

As requested.

hans accepted this revision.Jan 13 2020, 1:18 AM

Looks good to me (with a minor comment). Thanks!

clang/tools/driver/driver.cpp
271

I think just checking SpawnCC1Str == "0" or == "1" would be easier.

This revision is now accepted and ready to land.Jan 13 2020, 1:18 AM
This revision was automatically updated to reflect the committed changes.
aganea marked an inline comment as done.

Attention Apple maintainers: after this patch, do not forget to add -DCLANG_SPAWN_CC1=ON to your release scripts, to retain your previous crash reporting behavior (see discussion above). @dexonsmith @arphaman

Thanks for seeing this through!

env var or flag

Our build setup can easily add commandline flags, but it can't easily add env vars. So I think this should have a driver mode flag.

For people who asked for an env var: Why do you need an env var for this? Almost all compiler settings are accessible by flag but not by env var. Why is this one special?

This broke NetBSD buildbot:

+ : 'RUN: at line 1'
+ env -u CLANG_SPAWN_CC1 /home/motus/netbsd8/netbsd8/build-stage2/bin/clang -c /data/motus/netbsd8/netbsd8/llvm-project/clang/test/Driver/cc1-spawnprocess.c -o /dev/null
env: unknown option -- u
Usage: env [-i] [name=value ...] [command]

Please fix.

clang/test/Driver/cc1-spawnprocess.c
1

env -u is not portable.

joerg added a subscriber: joerg.Jan 14 2020, 7:27 AM
joerg added inline comments.
clang/test/Driver/cc1-spawnprocess.c
1

I think just going for env CLANG_SPAWN_CC1= works for the purpose of this test.

Trass3r removed a subscriber: Trass3r.Jan 14 2020, 8:55 AM

Hi!

This patch breaks scan-build-py which parses the output of "-###" to get -cc1 command. There might be other tools with the same problems. Could we either remove (in-process) from CC1Command::Print or add a line break?

Having the last line as a valid invocation is valuable and there might be tools relying on that.

NoQ added a comment.Jan 18 2020, 6:45 AM

This patch breaks scan-build-py which parses the output of "-###" to get -cc1 command.

Uh-oh, thanks for catching this. The same applies to the regular scan-build. Unfortunately, neither is properly tested. We really need to get this fixed.

@thakis landed a patch to control this behavior on the cmd-line, adding -fno-integrated-cc1 would solve your issue (ie. revert to old behavior). @NoQ @xazax.hun is that an acceptable solution?

There might be other tools parsing the output of -###. I think adding a line break in the output is better/easier than updating all those tools (amd making them potentially slower, not profiting from this patch).

wenlei added a subscriber: wenlei.Jan 18 2020, 11:48 PM

@aganea, @hans This patch broke response file expansion for preprocessor via -Wp - all our internal builds failed with this patch because we invoke clang with response file passed to preprocessor, e.g. clang++ ... -Wp,@pp.rsp @cc.rsp. Looks like we can't just call ExecuteCC1Tool directly, because response file expansion happens in main, so with this patch, -Wp,@pp.rsp won't be expanded properly.

repo:

CLANG_SPAWN_CC1=0  clang++ -c  -Wp,@a.rsp  @a.rsp test.cpp

error: error reading '@rr.rsp'
1 error generated.

and it works with CLANG_SPAWN_CC1=1

aganea added a comment.EditedJan 19 2020, 7:35 AM

@wenlei Taking a look now.
@hans I remember now why I was re-entering main() in the first iteration of this patch. Response files were one reason. And handling /MP was another, which triggers the re-entrance twice (once for CL driver, and once for cc1).

bjope added a subscriber: bjope.Jan 21 2020, 7:11 AM

When I build the clang on trunk, using clang 8.0 with asan enabled, and then run the clang lit tests I see lots of failures in Clang :: InterfaceStubs/driver-test.c and Clang :: InterfaceStubs/driver-test2.c (and maybe the faults I get in Clang :: Driver/cl-showfilenames.c comes from this patch as well).

The failures go away if I revert this patch (commit b4a99a061f517e60985667e39519f601). To make the revert I also needed to revert commit 8e5018e990b701391e6c33ba85b012343df67272 which is based on this patch.

Examples of failures:

==130643==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 192 byte(s) in 3 object(s) allocated from:
    #0 0xd0ea38 in operator new(unsigned long) /workspace/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:106
    #1 0x8153b36 in make_unique<clang::GenerateInterfaceIfsExpV1Action> /proj/flexasic/app/llvm/8.0/bin/../include/c++/v1/memory:3132:28
    #2 0x8153b36 in CreateFrontendBaseAction /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:68
    #3 0x8153b36 in clang::CreateFrontendAction(clang::CompilerInstance&) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:137
    #4 0x8156a22 in clang::ExecuteCompilerInvocation(clang::CompilerInstance*) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:287:39
    #5 0xd23349 in cc1_main(llvm::ArrayRef<char const*>, char const*, void*) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/tools/driver/cc1_main.cpp:239:15
    #6 0x7a0082e in operator() /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Driver/Job.cpp:402:34
    #7 0x7a0082e in void llvm::function_ref<void ()>::callback_fn<clang::driver::CC1Command::Execute(llvm::ArrayRef<llvm::Optional<llvm::StringRef> >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*, bool*) const::$_1>(long) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../include/llvm/ADT/STLExtras.h:108
    #8 0x6634feb in operator() /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../include/llvm/ADT/STLExtras.h:125:12
    #9 0x6634feb in llvm::CrashRecoveryContext::RunSafely(llvm::function_ref<void ()>) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../lib/Support/CrashRecoveryContext.cpp:396
    #10 0x79fefaf in clang::driver::CC1Command::Execute(llvm::ArrayRef<llvm::Optional<llvm::StringRef> >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*, bool*) const /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Driver/Job.cpp:402:12
    #11 0x795265d in clang::driver::Compilation::ExecuteCommand(clang::driver::Command const&, clang::driver::Command const*&) const /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Driver/Compilation.cpp:182:15
    #12 0x7953922 in clang::driver::Compilation::ExecuteJobs(clang::driver::JobList const&, llvm::SmallVectorImpl<std::__1::pair<int, clang::driver::Command const*> >&) const /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Driver/Compilation.cpp:233:19
    #13 0x79a4d0e in clang::driver::Driver::ExecuteCompilation(clang::driver::Compilation&, llvm::SmallVectorImpl<std::__1::pair<int, clang::driver::Command const*> >&) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Driver/Driver.cpp:1473:5
    #14 0xd19eac in main /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/tools/driver/driver.cpp:493:21
    #15 0x7f882be4e544 in __libc_start_main (/lib64/libc.so.6+0x22544)

Direct leak of 184 byte(s) in 1 object(s) allocated from:
    #0 0xd0ea38 in operator new(unsigned long) /workspace/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:106
    #1 0x81537b4 in make_unique<clang::EmitAssemblyAction> /proj/flexasic/app/llvm/8.0/bin/../include/c++/v1/memory:3132:28
    #2 0x81537b4 in CreateFrontendBaseAction /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:52
    #3 0x81537b4 in clang::CreateFrontendAction(clang::CompilerInstance&) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:137
    #4 0x8156a22 in clang::ExecuteCompilerInvocation(clang::CompilerInstance*) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:287:39
    #5 0xd23349 in cc1_main(llvm::ArrayRef<char const*>, char const*, void*) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/tools/driver/cc1_main.cpp:239:15
    #6 0x7a0082e in operator() /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Driver/Job.cpp:402:34
    #7 0x7a0082e in void llvm::function_ref<void ()>::callback_fn<clang::driver::CC1Command::Execute(llvm::ArrayRef<llvm::Optional<llvm::StringRef> >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*, bool*) const::$_1>(long) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../include/llvm/ADT/STLExtras.h:108
    #8 0x6634feb in operator() /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../include/llvm/ADT/STLExtras.h:125:12
    #9 0x6634feb in llvm::CrashRecoveryContext::RunSafely(llvm::function_ref<void ()>) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../lib/Support/CrashRecoveryContext.cpp:396
    #10 0x79fefaf in clang::driver::CC1Command::Execute(llvm::ArrayRef<llvm::Optional<llvm::StringRef> >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*, bool*) const /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Driver/Job.cpp:402:12
    #11 0x795265d in clang::driver::Compilation::ExecuteCommand(clang::driver::Command const&, clang::driver::Command const*&) const /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Driver/Compilation.cpp:182:15
    #12 0x7953922 in clang::driver::Compilation::ExecuteJobs(clang::driver::JobList const&, llvm::SmallVectorImpl<std::__1::pair<int, clang::driver::Command const*> >&) const /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Driver/Compilation.cpp:233:19
    #13 0x79a4d0e in clang::driver::Driver::ExecuteCompilation(clang::driver::Compilation&, llvm::SmallVectorImpl<std::__1::pair<int, clang::driver::Command const*> >&) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Driver/Driver.cpp:1473:5
    #14 0xd19eac in main /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/tools/driver/driver.cpp:493:21
    #15 0x7f882be4e544 in __libc_start_main (/lib64/libc.so.6+0x22544)

Indirect leak of 589860 byte(s) in 3 object(s) allocated from:
    #0 0xcd7ca7 in calloc /workspace/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:155
    #1 0x6694273 in safe_calloc /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../include/llvm/Support/MemAlloc.h:40:18
    #2 0x6694273 in llvm::StringMapImpl::init(unsigned int) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../lib/Support/StringMap.cpp:61
    #3 0xe4f90ee in StringMap /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../include/llvm/ADT/StringMap.h:251:7
    #4 0xe4f90ee in clang::IdentifierTable::IdentifierTable(clang::IdentifierInfoLookup*) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Basic/IdentifierTable.cpp:60
    #5 0xe49ab27 in clang::Preprocessor::Preprocessor(std::__1::shared_ptr<clang::PreprocessorOptions>, clang::DiagnosticsEngine&, clang::LangOptions&, clang::SourceManager&, clang::HeaderSearch&, clang::ModuleLoader&, clang::IdentifierInfoLookup*, bool, clang::TranslationUnitKind) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Lex/Preprocessor.cpp:92:7
    #6 0x7db6092 in __compressed_pair_elem<std::__1::shared_ptr<clang::PreprocessorOptions> &&, clang::DiagnosticsEngine &, clang::LangOptions &, clang::SourceManager &, clang::HeaderSearch &, clang::CompilerInstance &, nullptr_t &&, bool &&, clang::TranslationUnitKind &, 0, 1, 2, 3, 4, 5, 6, 7, 8> /proj/flexasic/app/llvm/8.0/bin/../include/c++/v1/memory:2156:9
    #7 0x7db6092 in __compressed_pair<std::__1::allocator<clang::Preprocessor> &, std::__1::shared_ptr<clang::PreprocessorOptions> &&, clang::DiagnosticsEngine &, clang::LangOptions &, clang::SourceManager &, clang::HeaderSearch &, clang::CompilerInstance &, nullptr_t &&, bool &&, clang::TranslationUnitKind &> /proj/flexasic/app/llvm/8.0/bin/../include/c++/v1/memory:2259
    #8 0x7db6092 in __shared_ptr_emplace<std::__1::shared_ptr<clang::PreprocessorOptions>, clang::DiagnosticsEngine &, clang::LangOptions &, clang::SourceManager &, clang::HeaderSearch &, clang::CompilerInstance &, nullptr_t, bool, clang::TranslationUnitKind &> /proj/flexasic/app/llvm/8.0/bin/../include/c++/v1/memory:3672
    #9 0x7db6092 in make_shared<std::__1::shared_ptr<clang::PreprocessorOptions>, clang::DiagnosticsEngine &, clang::LangOptions &, clang::SourceManager &, clang::HeaderSearch &, clang::CompilerInstance &, nullptr_t, bool, clang::TranslationUnitKind &> /proj/flexasic/app/llvm/8.0/bin/../include/c++/v1/memory:4331
    #10 0x7db6092 in make_shared<clang::Preprocessor, std::__1::shared_ptr<clang::PreprocessorOptions>, clang::DiagnosticsEngine &, clang::LangOptions &, clang::SourceManager &, clang::HeaderSearch &, clang::CompilerInstance &, nullptr_t, bool, clang::TranslationUnitKind &> /proj/flexasic/app/llvm/8.0/bin/../include/c++/v1/memory:4710
    #11 0x7db6092 in clang::CompilerInstance::createPreprocessor(clang::TranslationUnitKind) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Frontend/CompilerInstance.cpp:389
    #12 0x7ef58bf in clang::FrontendAction::BeginSourceFile(clang::CompilerInstance&, clang::FrontendInputFile const&) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Frontend/FrontendAction.cpp:744:8
    #13 0x7dc809f in clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Frontend/CompilerInstance.cpp:964:13
    #14 0x8156a4d in clang::ExecuteCompilerInvocation(clang::CompilerInstance*) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:290:25
    #15 0xd23349 in cc1_main(llvm::ArrayRef<char const*>, char const*, void*) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/tools/driver/cc1_main.cpp:239:15
    #16 0x7a0082e in operator() /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Driver/Job.cpp:402:34
    #17 0x7a0082e in void llvm::function_ref<void ()>::callback_fn<clang::driver::CC1Command::Execute(llvm::ArrayRef<llvm::Optional<llvm::StringRef> >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*, bool*) const::$_1>(long) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../include/llvm/ADT/STLExtras.h:108
    #18 0x6634feb in operator() /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../include/llvm/ADT/STLExtras.h:125:12
    #19 0x6634feb in llvm::CrashRecoveryContext::RunSafely(llvm::function_ref<void ()>) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../lib/Support/CrashRecoveryContext.cpp:396
    #20 0x79fefaf in clang::driver::CC1Command::Execute(llvm::ArrayRef<llvm::Optional<llvm::StringRef> >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*, bool*) const /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Driver/Job.cpp:402:12
    #21 0x795265d in clang::driver::Compilation::ExecuteCommand(clang::driver::Command const&, clang::driver::Command const*&) const /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Driver/Compilation.cpp:182:15
    #22 0x7953922 in clang::driver::Compilation::ExecuteJobs(clang::driver::JobList const&, llvm::SmallVectorImpl<std::__1::pair<int, clang::driver::Command const*> >&) const /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Driver/Compilation.cpp:233:19
    #23 0x79a4d0e in clang::driver::Driver::ExecuteCompilation(clang::driver::Compilation&, llvm::SmallVectorImpl<std::__1::pair<int, clang::driver::Command const*> >&) /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Driver/Driver.cpp:1473:5
    #24 0xd19eac in main /workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/tools/driver/driver.cpp:493:21
    #25 0x7f882be4e544 in __libc_start_main (/lib64/libc.so.6+0x22544)
smeenai added a subscriber: plotfi.Jan 21 2020, 9:37 AM

CC @plotfi for the InterfaceStubs failures

CC @plotfi for the InterfaceStubs failures

Thanks

CC @plotfi for the InterfaceStubs failures

@smeenai thanks for the CC. I have been looking into this asan failure and it isn't actually caused by the interface stubs AST consumer at all. What is triggering it is the pipeline setup. Here is what I gather:

  • Two standard cc1 CompileJobs followed by two Interface Stubs cc1 CompileJobs (or any other Job type for that matter): asan failure
  • if I clear the jobs list in Driver.cpp prior to appending the Interface Stubs jobs: No asan failure.
  • One standard cc1 CompileJob followed by one Interface Stubs/Alternate cc1 Compile job: No asan failure.

TL;DR
The InterfaceStubs ASTConsumer CompileJob is not causing the ASAN failure here, it's the setup of having a standard compilation pipeline followed by a secondary side-care pipeline thats triggering it in -fintegrated-cc1 mode. I am still looking into this and any help would be much appreciated. In the meantime I could -fno-integrated-cc1 the driver.c InterfaceStubs tests to quite the bots if that is what people want (and file a bug on bugzilla).

hctim added a subscriber: hctim.Jan 21 2020, 3:18 PM

In the meantime I could -fno-integrated-cc1 the driver.c InterfaceStubs tests to quite the bots if that is what people want (and file a bug on bugzilla).

Yes, please. A temporary workaround here that can be resolved later would be much appreciated to unblock our bots. Normally, I'd prefer a rollback-and-resubmit-with-fixes, but given that there have been multiple dependent changes submitted that would all have to be rolled back - whatever's the quickest way to get us back online without resorting to rolling back everything would be great :)

hctim added a comment.Jan 23 2020, 2:27 PM

For tracking purposes.

The ASan leaks have been worked around in two separate patches in order to make the bots green again:

@plotfi is graciously debugging :)

hans added a comment.Jan 23 2020, 2:41 PM

For tracking purposes.

The ASan leaks have been worked around in two separate patches in order to make the bots green again:

Thanks! I've cherry-picked these to the 10.x branch in 2dd6b91f35edb967f329f0437b53ea14395aa770 and bfaba51f07d1b79ff6e71da106c2b7e153874b1d

For tracking purposes.

The ASan leaks have been worked around in two separate patches in order to make the bots green again:

@plotfi is graciously debugging :)

How is the debugging going? If it takes a while to fix, maybe it's better to make -emit-interface-stub imply -fno-integrated-cc1 until it's fixed for real, instead of adding that in the tests? As-is, the tests are happy but the feature is likely broken (?)

I've filed this bug against this patch: https://bugs.llvm.org/show_bug.cgi?id=44823

We've discovered in cases where multiple TUs are invoked in a single command line that the memory between calls isn't cleaned up like it was previously. This results in really high memory use from a compilation.

Experimentation has shown that removing the addition of the -disable-free flag from the CC1Command command lines fixes the issue. However, I believe we should only remove that in cases where it is NOT the last CC1 command (since the disable-free has some pretty solid performance improvements).

plotfi added a comment.Feb 7 2020, 6:01 PM

@thakis I got sidetracked by some other things and had not made more progress, if progress is not made yes I think -emit-interface-stubs should imply -fno-integrated-cc1 if that seems like the right thing to do to you. @erichkeane Thanks for filing the bugzilla bug. I was having some problems with my bugzilla login myself and wasn't able to do so. I'll track progress if this bug and decide shortly if my feature should imply no-integraed-cc1. Thanks Guys.

For tracking purposes.

The ASan leaks have been worked around in two separate patches in order to make the bots green again:

@plotfi is graciously debugging :)

How is the debugging going? If it takes a while to fix, maybe it's better to make -emit-interface-stub imply -fno-integrated-cc1 until it's fixed for real, instead of adding that in the tests? As-is, the tests are happy but the feature is likely broken (?)

hans added a comment.Feb 10 2020, 7:02 AM

@erichkeane Thanks for filing the bugzilla bug. I was having some problems with my bugzilla login myself and wasn't able to do so. I'll track progress if this bug and decide shortly if my feature should imply no-integraed-cc1. Thanks Guys.

I think that bug was for the problem of high memory usage when doing multiple compilations in one clang invocation.

I've filed https://bugs.llvm.org/show_bug.cgi?id=44865 for the interface stubs vs asan issue to keep track of it easier. Please cc yourselves and track progress there.

Hello @plotfi and @hctim -- rG20f1abe306d0 should solve the initial issues you were seeing, please let me know if it doesn't.

On AIX and PPC LE Linux after this change we are seeing invalid accesses when the backend asserts/fatal_errors. Looks like the driver and CC1 now share some global TimerGroup state that points to Timers which got created by CC1 but didn't get cleanup after the assert. Reported as: https://bugs.llvm.org/show_bug.cgi?id=45164

Example failure:

./bin/clang ./llvm-project/clang/test/CodeGenObjC/illegal-UTF8.m -S -target powerpc-ibm-aix
...
fatal error: error in backend: Unhandled linkage when mapping linkage to StorageClass.
Stack dump:
...
*******************
AddressSanitizer:DEADLYSIGNAL
=================================================================
==37262==ERROR: AddressSanitizer: SEGV on unknown address 0x000010715768 (pc 0x3fffb7cdb2c4 bp 0x3e1fb764cdb0 sp 0x3fffffffc460 T0)
==37262==The signal is caused by a UNKNOWN memory access.
    #0 0x3fffb7cdb2c0 in std::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string(std::string const&) (/lib64/libstdc++.so.6+0x11b2c0)
    #1 0x16ca75c0 in PrintRecord /home/daltenty/llvm/dev/llvm-project/llvm/include/llvm/Support/Timer.h:180:21
    #2 0x16ca75c0 in construct<llvm::TimerGroup::PrintRecord, llvm::TimeRecord &, std::basic_string<char> &, std::basic_string<char> &> /opt/rh/devtoolset-7/root/usr/lib/gcc/ppc64le-redhat-linux/7/../../../../include/c++/7/ext/new_allocator.h:136:23
    #3 0x16ca75c0 in construct<llvm::TimerGroup::PrintRecord, llvm::TimeRecord &, std::basic_string<char> &, std::basic_string<char> &> /opt/rh/devtoolset-7/root/usr/lib/gcc/ppc64le-redhat-linux/7/../../../../include/c++/7/bits/alloc_traits.h:475:8
    #4 0x16ca75c0 in void std::vector<llvm::TimerGroup::PrintRecord, std::allocator<llvm::TimerGroup::PrintRecord> >::_M_realloc_insert<llvm::TimeRecord&, std::string&, std::string&>(__gnu_cxx::__normal_iterator<llvm::TimerGroup::PrintRecord*, std::vector<llvm::TimerGroup::PrintRecord, std::allocator<llvm::TimerGroup::PrintRecord> > >, llvm::TimeRecord&, std::string&, std::string&) /opt/rh/devtoolset-7/root/usr/lib/gcc/ppc64le-redhat-linux/7/../../../../include/c++/7/bits/vector.tcc:415:4
    #5 0x16ca2220 in emplace_back<llvm::TimeRecord &, std::basic_string<char> &, std::basic_string<char> &> /opt/rh/devtoolset-7/root/usr/lib/gcc/ppc64le-redhat-linux/7/../../../../include/c++/7/bits/vector.tcc:105:4
    #6 0x16ca2220 in llvm::TimerGroup::prepareToPrintList(bool) /home/daltenty/llvm/dev/llvm-project/llvm/lib/Support/Timer.cpp:359:19
    #7 0x16ca2498 in llvm::TimerGroup::print(llvm::raw_ostream&, bool) /home/daltenty/llvm/dev/llvm-project/llvm/lib/Support/Timer.cpp:373:5
    #8 0x16ca2a4c in llvm::TimerGroup::printAll(llvm::raw_ostream&) /home/daltenty/llvm/dev/llvm-project/llvm/lib/Support/Timer.cpp:391:9
    #9 0x107f1940 in main /home/daltenty/llvm/dev/llvm-project/clang/tools/driver/driver.cpp:535:3
    #10 0x3fffb79b497c in generic_start_main.isra.0 (/lib64/libc.so.6+0x2497c)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/lib64/libstdc++.so.6+0x11b2c0) in std::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string(std::string const&)
==37262==ABORTING

On AIX and PPC LE Linux after this change we are seeing invalid accesses when the backend asserts/fatal_errors. Looks like the driver and CC1 now share some global TimerGroup state that points to Timers which got created by CC1 but didn't get cleanup after the assert. Reported as: https://bugs.llvm.org/show_bug.cgi?id=45164

From the bug report, it looks like we are discovering more problems associated with this patch that affect the 10.0 release. I've marked the bug as blocking release-10.0.0 for triage purposes for now.

Looking at the issue now, it should be a straightforward fix. Will send a patch ASAP.

dim added a subscriber: dim.May 29 2020, 12:49 PM

FWIW, this change causes https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=246630, see in particular https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=246630#c18.

For some reason, not spawning a new process for the cc1 stage can lead to non-reproducible output. The exact cause is not known yet.

hans added a comment.Jun 3 2020, 4:33 AM
In D69825#2063979, @dim wrote:

FWIW, this change causes https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=246630, see in particular https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=246630#c18.

For some reason, not spawning a new process for the cc1 stage can lead to non-reproducible output. The exact cause is not known yet.

Can you share the preprocessed source and compiler command-line for printf.c, either on the bug or here?

dim added a comment.Jun 3 2020, 9:30 AM
In D69825#2063979, @dim wrote:

FWIW, this change causes https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=246630, see in particular https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=246630#c18.

For some reason, not spawning a new process for the cc1 stage can lead to non-reproducible output. The exact cause is not known yet.

Can you share the preprocessed source and compiler command-line for printf.c, either on the bug or here?

See https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=246630#c26, and I've also uploaded the repro files to https://www.andric.com/freebsd/clang/bug246630-repro.tar.xz.

Running rGa3220dffcb1 under valgrind (on Linux) shows:

==120363== Conditional jump or move depends on uninitialised value(s)
==120363==    at 0x1634474: llvm::ConstantExpr::getGetElementPtr(llvm::Type*, llvm::Constant*, llvm::ArrayRef<llvm::Value*>, bool, llvm::Optional<unsigned int>, llvm::Type*) (Constants.cpp:2191)
==120363==    by 0x112D6D9: getGetElementPtr (Constants.h:1163)
==120363==    by 0x112D6D9: (anonymous namespace)::SymbolicallyEvaluateGEP(llvm::GEPOperator const*, llvm::ArrayRef<llvm::Constant*>, llvm::DataLayout const&, llvm::TargetLibraryInfo const*) (ConstantFolding.cpp:1005)
==120363==    by 0x112DF70: (anonymous namespace)::ConstantFoldInstOperandsImpl(llvm::Value const*, unsigned int, llvm::ArrayRef<llvm::Constant*>, llvm::DataLayout const&, llvm::TargetLibraryInfo const*) (ConstantFolding.cpp:1039)
==120363==    by 0x112C165: (anonymous namespace)::ConstantFoldConstantImpl(llvm::Constant const*, llvm::DataLayout const&, llvm::TargetLibraryInfo const*, llvm::SmallDenseMap<llvm::Constant*, llvm::Constant*, 4u, llvm::DenseMapInfo<llvm::Constant*>, llvm::detail::DenseMapPair<llvm::Constant*, llvm::Constant*> >&) [clone .part.0] (ConstantFolding.cpp:1114)
==120363==    by 0x112C5CF: llvm::ConstantFoldConstant(llvm::Constant const*, llvm::DataLayout const&, llvm::TargetLibraryInfo const*) (ConstantFolding.cpp:1194)
==120363==    by 0x188F410: prepareICWorklistFromFunction (InstructionCombining.cpp:3584)
==120363==    by 0x188F410: combineInstructionsOverFunction(llvm::Function&, llvm::InstCombineWorklist&, llvm::AAResults*, llvm::AssumptionCache&, llvm::TargetLibraryInfo&, llvm::DominatorTree&, llvm::OptimizationRemarkEmitter&, llvm::BlockFrequencyInfo*, llvm::ProfileSummaryInfo*, unsigned int, llvm::LoopInfo*) (InstructionCombining.cpp:3703)
==120363==    by 0x189205F: runOnFunction (InstructionCombining.cpp:3789)
==120363==    by 0x189205F: llvm::InstructionCombiningPass::runOnFunction(llvm::Function&) (InstructionCombining.cpp:3768)
==120363==    by 0x16F4352: llvm::FPPassManager::runOnFunction(llvm::Function&) (LegacyPassManager.cpp:1482)
==120363==    by 0x16F4DE8: llvm::FPPassManager::runOnModule(llvm::Module&) (LegacyPassManager.cpp:1518)
==120363==    by 0x16F51A2: runOnModule (LegacyPassManager.cpp:1583)
==120363==    by 0x16F51A2: llvm::legacy::PassManagerImpl::run(llvm::Module&) (LegacyPassManager.cpp:1695)
==120363==    by 0x1FF4CFE: EmitAssembly (BackendUtil.cpp:954)
==120363==    by 0x1FF4CFE: clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::DataLayout const&, llvm::Module*, clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) (BackendUtil.cpp:1677)
==120363==    by 0x2C471A8: clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) (CodeGenAction.cpp:335)
==120363==  Uninitialised value was created by a stack allocation
==120363==    at 0x112C653: (anonymous namespace)::SymbolicallyEvaluateGEP(llvm::GEPOperator const*, llvm::ArrayRef<llvm::Constant*>, llvm::DataLayout const&, llvm::TargetLibraryInfo const*) (ConstantFolding.cpp:832)

Trying to reduce this now.

dim added a comment.Jun 4 2020, 10:56 AM
In D69825#2071687, @dim wrote:

Trying to reduce this now.

Unfortunately this turned out to be a red herring, as the test case got reduced by creduce to just:

a() { b(""); }

The valgrind warnings are also different when you build clang with clang, instead of gcc as I did first. In that case, you get dozens of these instead:

==525651== Conditional jump or move depends on uninitialised value(s)
==525651==    at 0xBB1D71: SimplifyAndInst(llvm::Value*, llvm::Value*, llvm::SimplifyQuery const&, unsigned int) (in /home/dim/obj/llvm-llvmorg-10.0.0-53-gf79cd71e145-linux5-x86_64-ninja-rel-1/bin/clang-10)
==525651==    by 0xBBF6D0: ThreadBinOpOverPHI(llvm::Instruction::BinaryOps, llvm::Value*, llvm::Value*, llvm::SimplifyQuery const&, unsigned int) (in /home/dim/obj/llvm-llvmorg-10.0.0-53-gf79cd71e145-linux5-x86_64-ninja-rel-1/bin/clang-10)
==525651==    by 0xBB1F87: SimplifyAndInst(llvm::Value*, llvm::Value*, llvm::SimplifyQuery const&, unsigned int) (in /home/dim/obj/llvm-llvmorg-10.0.0-53-gf79cd71e145-linux5-x86_64-ninja-rel-1/bin/clang-10)
==525651==    by 0x13898E8: llvm::InstCombiner::visitAnd(llvm::BinaryOperator&) (in /home/dim/obj/llvm-llvmorg-10.0.0-53-gf79cd71e145-linux5-x86_64-ninja-rel-1/bin/clang-10)
==525651==    by 0x1365A6D: llvm::InstCombiner::run() (in /home/dim/obj/llvm-llvmorg-10.0.0-53-gf79cd71e145-linux5-x86_64-ninja-rel-1/bin/clang-10)
==525651==    by 0x1367AEC: combineInstructionsOverFunction(llvm::Function&, llvm::InstCombineWorklist&, llvm::AAResults*, llvm::AssumptionCache&, llvm::TargetLibraryInfo&, llvm::DominatorTree&, llvm::OptimizationRemarkEmitter&, llvm::BlockFrequencyInfo*, llvm::ProfileSummaryInfo*, bool, unsigned int, llvm::LoopInfo*) (in /home/dim/obj/llvm-llvmorg-10.0.0-53-gf79cd71e145-linux5-x86_64-ninja-rel-1/bin/clang-10)
==525651==    by 0x1369369: llvm::InstructionCombiningPass::runOnFunction(llvm::Function&) (in /home/dim/obj/llvm-llvmorg-10.0.0-53-gf79cd71e145-linux5-x86_64-ninja-rel-1/bin/clang-10)
==525651==    by 0x121015B: llvm::FPPassManager::runOnFunction(llvm::Function&) (in /home/dim/obj/llvm-llvmorg-10.0.0-53-gf79cd71e145-linux5-x86_64-ninja-rel-1/bin/clang-10)
==525651==    by 0x3BA5188: (anonymous namespace)::CGPassManager::runOnModule(llvm::Module&) (in /home/dim/obj/llvm-llvmorg-10.0.0-53-gf79cd71e145-linux5-x86_64-ninja-rel-1/bin/clang-10)
==525651==    by 0x1210B3F: llvm::legacy::PassManagerImpl::run(llvm::Module&) (in /home/dim/obj/llvm-llvmorg-10.0.0-53-gf79cd71e145-linux5-x86_64-ninja-rel-1/bin/clang-10)
==525651==    by 0x19A503A: clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::DataLayout const&, llvm::Module*, clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) (in /home/dim/obj/llvm-llvmorg-10.0.0-53-gf79cd71e145-linux5-x86_64-ninja-rel-1/bin/clang-10)
==525651==    by 0x2500BD4: clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) (in /home/dim/obj/llvm-llvmorg-10.0.0-53-gf79cd71e145-linux5-x86_64-ninja-rel-1/bin/clang-10)

I'm unsure how to continue.

dim added a comment.Jun 4 2020, 1:37 PM

For completeness sake, here are the valgrind outputs of clang rGf7f1abdb889 (llvmorg-11-init-16778) built by gcc 9.3.0-10ubuntu2 (in RelWithDebInfo mode):

And the exact same version of clang, built by clang 10.0.0-4ubuntu1:

.

dim added a subscriber: asbirlea.Jun 13 2020, 3:31 PM

Okay, after a long while of attempting to make a reproduction scenario, I finally managed one that consistently worked. As BSD make puts in different redirections for stderr when running in -j mode, it turned out that I could simply run the compilation twice on a terminal, first normally, then with stderr redirected to /dev/null, e.g. my test script is:

#!/bin/sh

exec </dev/tty >/dev/tty 2>&1

ulimit -c 0
trap "rm -f testcase[12].s testcase[12].s-* testcase-*.s.tmp" EXIT

${CLANG:-clang} -fintegrated-cc1 -Werror -O2 -std=gnu99 -fstack-protector-strong -S testcase.c -o testcase1.s || exit 1
${CLANG:-clang} -fintegrated-cc1 -Werror -O2 -std=gnu99 -fstack-protector-strong -S testcase.c -o testcase2.s 2>/dev/null || exit 1

cmp -s testcase1.s testcase2.s
test $? = 1 || exit 1

exit 0

(Here testcase.c is a copy of the .c file in https://www.andric.com/freebsd/clang/bug246630-repro.tar.xz)

Using this script I could finally bisect, and arrived at the following commit which fixes the issue, i.e. it makes the outputs of both clang runs the same again, and it is rG0cecafd647cc ("[BasicAA] Make BasicAA a cfg pass"). @asbirlea seems to indicate in the commit message that it influences the way the BasicAA pass is freed/invalidaed or not during runs?

So is this commit fixing it as an unintended side effect, or would it be according to expectation? If the latter, we should merge it into 10.0.1, and I will create a PR for it.