• fix two linux test failure

ping?

Somehow this caused three failures on Linux. I'll take a look.

• enable profile test-suite discovery

In D73010#1828890, @MaskRay wrote:

I tend to ignore failures on clang-ppc64be-linux before... From my experience it was quite unstable and often failed with c++: internal compiler error: Killed (program cc1plus). Not sure if it has improved. If it is the only failing bot I tend to ignore it. Maybe we can ask the admin of the build bot.

In D73010#1828880, @evgeny777 wrote:

Honestly, this looks weird (why 2 threads not 1 for example)?

1 feels very special for multithreading code. I've seen code with separate paths for 1 and > 1, although I'm not aware of any similar code for ThinLTO.

@MaskRay @mehdi_amini I have no clue either on how this does not happen before. Adding std::__throw_system_error in ThreadPool ctor and reverting this change, I still see the crash in my machine (Ubuntu 18.04). I'm inclined to think the exception did not trigger before rather than it was triggered but somehow muted in some way.

@MaskRay Yeah, that would be ideal. I'm also in favor of supporting python3 only. The change for open to add encoding parameter is still needed though since the default value is platform-dependent.

There are some interesting failures that need to be investigated.
https://reviews.llvm.org/rG647c3f4e47de8a850ffcaa897db68702d8d2459a

This is going deeper...

I saw a null pointer dereference for the p3-2a.cpp file added here. It is not always reproducible for me, I was able to reproduce it once offline. Any idea?

• fix tests

Apologies that it takes so long. I'll fix the test and land it today.

• rebase

• fix test when clang is the host compiler

In D72404#1820628, @tycho wrote:

In D72404#1820461, @merge_guards_bot wrote:

Unit tests: fail. 61858 tests passed, 1 failed and 781 were skipped.

failed: Clang.CodeGen/thinlto-debug-pm.c

I think that failure can be fixed with something like this:

diff --git a/clang/test/CodeGen/thinlto-debug-pm.c b/clang/test/CodeGen/thinlto-debug-pm.c
 index 5f449d493af..9d6d69afd13 100644
 --- a/clang/test/CodeGen/thinlto-debug-pm.c
 +++ b/clang/test/CodeGen/thinlto-debug-pm.c
 @@ -13,17 +13,17 @@
  // O0123sz-NEWPM: Running analysis: PassInstrumentationAnalysis
  // O0123sz-NEWPM: Starting llvm::Module pass manager run.
  // O0123sz-NEWPM: Running pass: WholeProgramDevirtPass
 -// O0123sz-NEWPM: Running analysis: InnerAnalysisManagerProxy<llvm::AnalysisManager<llvm::Function>, llvm::Module>
 +// O0123sz-NEWPM: Running analysis: InnerAnalysisManagerProxy<llvm::FunctionAnalysisManager, llvm::Module>
  // O0123sz-NEWPM: Running pass: LowerTypeTestsPass
  // O0123sz-NEWPM: Invalidating all non-preserved analyses for:
 -// O0123sz-NEWPM: Invalidating analysis: InnerAnalysisManagerProxy<llvm::AnalysisManager<llvm::Function>, llvm::Module>
 +// O0123sz-NEWPM: Invalidating analysis: InnerAnalysisManagerProxy<llvm::FunctionAnalysisManager, llvm::Module>
  // O123sz-NEWPM: Running pass: ForceFunctionAttrsPass
  // O123sz-NEWPM: Running pass: PassManager<llvm::Module>
  // O123sz-NEWPM: Starting llvm::Module pass manager run.
  // O123sz-NEWPM: Running pass: PGOIndirectCallPromotion
  // O123sz-NEWPM: Running analysis: ProfileSummaryAnalysis
  // O123sz-NEWPM: Running pass: InferFunctionAttrsPass
 -// O123sz-NEWPM: Running analysis: InnerAnalysisManagerProxy<llvm::AnalysisManager<llvm::Function>, llvm::Module>
 +// O123sz-NEWPM: Running analysis: InnerAnalysisManagerProxy<llvm::FunctionAnalysisManager, llvm::Module>
  // O123sz-NEWPM: Running pass: ModuleToFunctionPassAdaptor<llvm::PassManager<llvm::Function> >
  // O123sz-NEWPM: Running analysis: PassInstrumentationAnalysis on foo
  // O123sz-NEWPM: Starting llvm::Function pass manager run.
 @@ -41,9 +41,9 @@
  // O123sz-NEWPM: Running pass: CalledValuePropagationPass
  // O123sz-NEWPM: Running pass: GlobalOptPass
  // O123sz-NEWPM: Invalidating all non-preserved analyses for:
 -// O123sz-NEWPM: Invalidating analysis: InnerAnalysisManagerProxy<llvm::AnalysisManager<llvm::Function>, llvm::Module>
 +// O123sz-NEWPM: Invalidating analysis: InnerAnalysisManagerProxy<llvm::FunctionAnalysisManager, llvm::Module>
  // O123sz-NEWPM: Running pass: ModuleToFunctionPassAdaptor<llvm::PromotePass>
 -// O123sz-NEWPM: Running analysis: InnerAnalysisManagerProxy<llvm::AnalysisManager<llvm::Function>, llvm::Module>
 +// O123sz-NEWPM: Running analysis: InnerAnalysisManagerProxy<llvm::FunctionAnalysisManager, llvm::Module>
  // O123sz-NEWPM: Running analysis: DominatorTreeAnalysis on foo
  // O123sz-NEWPM: Running analysis: PassInstrumentationAnalysis on foo
  // O123sz-NEWPM: Running analysis: AssumptionAnalysis on foo
 @@ -57,7 +57,7 @@
  // O123sz-NEWPM: Running analysis: BasicAA on foo
  // O123sz-NEWPM: Running analysis: ScopedNoAliasAA on foo
  // O123sz-NEWPM: Running analysis: TypeBasedAA on foo
 -// O123sz-NEWPM: Running analysis: OuterAnalysisManagerProxy<llvm::AnalysisManager<llvm::Module>, llvm::Function> on foo
 +// O123sz-NEWPM: Running analysis: OuterAnalysisManagerProxy<llvm::ModuleAnalysisManager, llvm::Function> on foo
  // O123sz-NEWPM: Running pass: SimplifyCFGPass on foo
  // O123sz-NEWPM: Running analysis: TargetIRAnalysis on foo
  // O123sz-NEWPM: Finished llvm::Function pass manager run.
 @@ -70,7 +70,7 @@
  // O123sz-NEWPM: Running analysis: LazyCallGraphAnalysis
  // O123sz-NEWPM: Running analysis: FunctionAnalysisManagerCGSCCProxy on (foo)
  // O123sz-NEWPM: Running analysis: PassInstrumentationAnalysis on (foo)
 -// O123sz-NEWPM: Running analysis: OuterAnalysisManagerProxy<llvm::AnalysisManager<llvm::Module>, llvm::LazyCallGraph::SCC, llvm::LazyCallG>
 +// O123sz-NEWPM: Running analysis: OuterAnalysisManagerProxy<llvm::ModuleAnalysisManager, llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&> o>
  // O123sz-NEWPM: Starting CGSCC pass manager run.
  // O123sz-NEWPM: Running pass: InlinerPass on (foo)
  // O123sz-NEWPM: Running pass: PostOrderFunctionAttrsPass on (foo)

• rebase & clang-format

• Rebase

• fix a typo

• when optnone is not present, add optsize for Os and minsize for Oz
• add Os Oz function attribute test.
• add Os Oz pipeline test

Since this is -fapple-ext specific, would it be better to keep everything as is and move the always_inline setting for -fapple-ext from code emission to after the regular inline attribute decision is made on non--fapple-ext cases in SetLLVMFunctionAttributesForDefinition? It would be less diff and keep the assertion which IMHO is still useful.

fix test

• fix test

In D72404#1810489, @pcc wrote:

This is going in the wrong direction IMO. We already have a way of specifying the size optimization level, which is to specify -Os or -Oz at compile time. Why is that not sufficient?

--cmake-define works just fine.

In D71684#1803110, @Meinersbur wrote:

In D71684#1793685, @ychen wrote:

Thanks for the feedback. Obviously, my CMake skills needs to catch up. My
intention was to add the macro definition only for CXX compilation, but
add_definitions, add_compile_options change both C and CXX flags AFAIK at
the time.

Does defining __BOOL_DEFINED for C as well have any effect? If not, I'd define it for both languages for simplicity.

Defining __BOOL_DEFINED for C would cause build error.

address the comment

LGTM

Probably need to translate old attributes to new attribute in AutoUpgrade.cpp? Otherwise, this may cause problems for existing LTO libraries.

Thanks for the feedback. Obviously, my CMake skills needs to catch up. My
intention was to add the macro definition only for CXX compilation, but
add_definitions, add_compile_options change both C and CXX flags AFAIK at
the time. After some digging, it seems add_compile_options and
add_compile_definitions support cmake-generator-expressions that could be
used to implement this perfectly. However, add_compile_definitions was not
available until CMake v3.12, so I use add_compile_options here.

In D71261#1786331, @steven_wu wrote:

This sounds like a good idea. Have you thought about just write out upgraded bitcode file on disk (since you named it a "SymTabCache"), maybe into thinLTO cache? This will speed up the incremental build as well. I think most of the upgrade path are coming from linking a bitcode static library, which is probably not changing during incremental build so we just need to upgrade once.

fix a bug

Address reviewer's comments.

Thank you @steven_wu @tejohnson. I created D69406 to promote the flag to the driver.

Sorry for the confusion @steven_wu. By stable I mean the probability that the -fthin-link-bitcode option is replaced with some other thinlink mechanism under the distributed build environment. Since at least for ccache, the compilation output caching depends on the semantics of options ("-o" is assumed to be compilation output). For the case of -fthin-link-bitcode, both -Xclang -fthin-link-bitcode and -o are the output. I'm not familiar with compiler cache tools, but having the caching depends on a cc1 option feels not right since it is not an option of any other compilers, so most caching tools don't recognize it.

In D69327#1719419, @steven_wu wrote:

In D69327#1719411, @ychen wrote:

Thanks for the inputs @steven_wu @tejohnson. Totally agree with the points you brought up. One last thing I'm not quite sure is the caching of -fthin-link-bitcode. It is a -cc1 option since it is a kind of implementation of ThinLTO, right? I'm a little hesitant to begin writing up patches to teach build system/caching tool (I could think of at least three for our workload) to recognize this option because of that. If there are any changes to the option, the same thing needs to be done again. Do you have any thoughts on that? Is the option in use for your workload and do you think it is stable enough to have build systems caching for it? (Another option is to produce -fthin-link-bitcode output post compile time which I assume having total build time impact to some degree).

-fthin-link-bitcode option is used to run distributed thin link. The format is not stable but it is deterministic for a fixed compiler version. You should be able to cache the thin-link-bitcode and expected it to be used only by the same compiler version.

For any build system that implements caching, it must take compiler version into consideration because different compiler will produce different output. I don't think the rule to cache thin-link-bitcode is any different from any other output during the build.

Thanks for the inputs @steven_wu @tejohnson. Totally agree with the points you brought up. One last thing I'm not quite sure is the caching of -fthin-link-bitcode. It is a -cc1 option since it is a kind of implementation of ThinLTO, right? I'm a little hesitant to begin writing up patches to teach build system/caching tool (I could think of at least three for our workload) to recognize this option because of that. If there are any changes to the option, the same thing needs to be done again. Do you have any thoughts on that? Is the option in use for your workload and do you think it is stable enough to have build systems caching for it? (Another option is to produce -fthin-link-bitcode output post compile time which I assume having total build time impact to some degree).

In D69327#1719109, @tejohnson wrote:

I haven't read through the patch in detail yet, but from the description it sounds like a cache is being added for the compile step outputs, e.g. the bitcode output object of the "clang -flto=thin -c" step? Typically the build system takes care of incremental build handling for these explicit outputs, just as it would for a non-LTO clang compile output. Can you clarify the motivation?

In D67847#1691898, @jyknight wrote:

The abort() function raises SIGABRT, for which the default behavior is to trigger a coredump. Do we actually want that behavior?

Either _exit() (long available extension, which lld already uses) or quick_exit() (the new C standard way) seem possibly preferable?

Fix tests

In D68164#1689164, @MaskRay wrote:

In D68164#1689162, @ychen wrote:

In D68164#1689155, @MaskRay wrote:

The exit is called twice and somehow the test exit cleanly.

The current behavior is undefined. POSIX says "If exit() is called more than once, the behavior is undefined." Changing exit to abort in D67847 will fix the issue. Thanks.

The test ; RUN: sh -c 'opt --reject-this-option 2>&-; echo $?; opt -o /dev/null /dev/null 2>&-; echo $?;' is testing an undefined behavior, but I can see 2>&- is a simple alternative to a full-fledged test that makes write operations fail with some other errno. My feeling is that you can just update close-stderr.ll to (not --crash opt --reject-this-option 2>&-) in D67847 and be prepared to update it if build bots on some platforms have different behaviors.

sorry, I'm really confused now. Based on the comment in close-stderr.ll, the test is to make sure abort does not happen? Why is it checking the undefined behavior?

close-stderr.ll was added to test a particular behavior of an undefined behavior.

By undefined I don't know what kind of consistent behavior could be checked.

In D68164#1689155, @MaskRay wrote:

The exit is called twice and somehow the test exit cleanly.

The current behavior is undefined. POSIX says "If exit() is called more than once, the behavior is undefined." Changing exit to abort in D67847 will fix the issue. Thanks.

The test ; RUN: sh -c 'opt --reject-this-option 2>&-; echo $?; opt -o /dev/null /dev/null 2>&-; echo $?;' is testing an undefined behavior, but I can see 2>&- is a simple alternative to a full-fledged test that makes write operations fail with some other errno. My feeling is that you can just update close-stderr.ll to (not --crash opt --reject-this-option 2>&1) in D67847 and be prepared to update it if build bots on some platforms have different behaviors.

In D68164#1689152, @ychen wrote:

In D68164#1689148, @ruiu wrote:

So, could you add a testcase to demonstrate the issue?

If an error occurs, I believe this piece of code will try to call report_fatal_error and that will fail too for the same reason (because if the code fails, it is likely that we can't write anything to stderr), and the process will exit with exit code 1. So, is the new behavior implemented in this patch distinguishable from the existing behavior? If a command behaves differently with this patch, please add a test to demonstrate it.

The new behavior is not distinguishable from the existing behavior from the user's perspective. The existing behavior does exit(1)-> report_fatal_error -> exit(1) and new behavior just exit(1) cleanly.

In D68164#1689148, @ruiu wrote:

So, could you add a testcase to demonstrate the issue?

If an error occurs, I believe this piece of code will try to call report_fatal_error and that will fail too for the same reason (because if the code fails, it is likely that we can't write anything to stderr), and the process will exit with exit code 1. So, is the new behavior implemented in this patch distinguishable from the existing behavior? If a command behaves differently with this patch, please add a test to demonstrate it.

In D68164#1687403, @ruiu wrote:

Can you add a testcase to demonstrate the issue?

Removed in r373297. Thanks.

Fixed in r373244. Thanks!

In D68164#1687442, @ruiu wrote:

In D68164#1687414, @MaskRay wrote:

POSIX has stated that file descriptors 0, 1 or 2 should not be closed. The is stated in many parts, e.g.

Applications should not execute programs with file descriptor 0 not open for reading or with file descriptor 1 or 2 not open for writing, as this might cause the executed program to misbehave. In order not to pass on these file descriptors to an executed program, applications should not just close them but should reopen them on, for example, /dev/null.

The change is justified if you can give an example of a different file descriptor.

I believe that sentence is talking about stdin/stdout/stderr and forking a new process. As long as you don't create a new process, I think you are free to close 0, 1 and 2.

That said I agree with you that we shouldn't close 0, 1 and 2 on exit. I'm not sure if we are doing this at the moment, but if we do, I think we should stop doing that.

In D67847#1677251, @rnk wrote:

This has been proposed before by @MatzeB:
https://reviews.llvm.org/D33960

At the time, there was an issue that some people had been "fixing" fuzzer bugs by replacing asserts with if (cond) report_fatal_error(...);. Moving to abort will reopen all of those fuzzer bugs. However, I think we have consensus that that is the desired behavior: every call to report_fatal_error is essentially a bug,. Code should be using the error handlers on MCContext and LLVMContext instead and recovering.

Here is another case showing the issue http://lists.llvm.org/pipermail/llvm-dev/2019-September/135283.html

update

Pass in instead of manage IR unit analysis managers in MachineFunctionIRAnalysisManager

Hello, Charlie went back to school after the internship. I'm trying the address the comments in a new patch. https://reviews.llvm.org/D67687

• Missing assignment of TheModule in move constructor.

ping?