Add an -fno-temp-file flag for compilation

[zahen]

Summary

Our build system does not handle randomly named files created during the build well. We'd prefer to write compilation output directly without creating a temporary file. Function parameters already existed to control this behavior but were not exposed all the way out to the command line.

I'm open to suggestions what kinds of tests could be added alongside the change. Anecdotally, this code been running in internal production builds for months.

I do not have commit access so I will need someone else to submit the patch once it's accepted.

[rnk]

+more reviewers

This doesn't add any code complexity, we already have the boolean UseTempFile flag, so I think we should do this. I would also point out that right now, in my LLVM build directory on Windows, I have 895 *.obj.tmp files:

$ find . -iname '*.obj.tmp' | wc -l
895

This is, to say the least, annoying.

We've tried in the past to do better at cleaning these up, but I think we might want to give up and just open the object for writing, at least on Windows, where it usually locks the file.

[hans]

Seems fine to me. I'm not sure how to test the actual "don't write to temp file" functionality, but at least there could be a test to check that the flag gets forwarded to cc1.

[thakis]

In D70615#1769751, @rnk wrote:

+more reviewers

This doesn't add any code complexity, we already have the boolean UseTempFile flag, so I think we should do this. I would also point out that right now, in my LLVM build directory on Windows, I have 895 *.obj.tmp files:

$ find . -iname '*.obj.tmp' | wc -l
895

This is, to say the least, annoying.

We've tried in the past to do better at cleaning these up, but I think we might want to give up and just open the object for writing, at least on Windows, where it usually locks the file.

The reason the temp file is used (I'm guessing) is that if the compiler crashes while writing output, it'll now write a partial file. This will confuse build systems (both mtime and hash-based ones). If we add this, it should have a "this will lead to incorrect incremental builds" disclaimer in a prominent place (e.g. the help text).

[zahen]

Updated with review feedback and formatting fixes

[zahen]

Fixed patch

[zahen]

In D70615#1770550, @hans wrote:

Seems fine to me. I'm not sure how to test the actual "don't write to temp file" functionality, but at least there could be a test to check that the flag gets forwarded to cc1.

Added a best guess on the flag forwarding test case

[hans]

Our build system does not handle randomly named files created during the build well.

Can you share more details about why this is a problem? At least the build systems I work with would not even see the temporary files, as they're written to /tmp or similar, and instead only see either the finished object file on success, or no file on failure -- which as thakis points out is an important property.

[zahen]

In our build system every file access in an "output directory" needs to be accounted for. Until this patch, the random temporary file name has forced us to rely on workarounds that hurt build throughput.

I've uploaded example ProcMon dumps of clang-cl.exe file accesses to help illustrate what the patch is trying to achieve. In the RTM log, clang-cl creates a file ConsoleApplication1-dc91d1ce.obj.tmp directly in the output directory, writes the contents and ends by calling SetRenameInformationFile to "create" the final ConsoleApplication1.obj. In the patched log the file ConsoleApplication1.obj is created and written to directly.

clang9.0-RTM.CSV21 KBDownload

clang9.0-patched.CSV17 KBDownload

[hans]

I see. For some reason I thought the temporary files would be written elsewhere, like in /tmp, but I see that's not the case, and I guess it also makes sense to avoid having to copy it between file systems.

This patch seems okay to me.

[zahen]

Thanks Hans! I'll need someone to do the actual submission since I don't have commit rights.

[zahen]

Any additional changes required? If not could someone please submit on my behalf? @rnk, @hans, @thakis ?

[hans]

In D70615#1789142, @zahen wrote:

Any additional changes required? If not could someone please submit on my behalf? @rnk, @hans, @thakis ?

Sorry, this got lost in my email. I've committed it as d129aa1d5369781deff6c6b854cb612e160d3fb2.

[hctim]

This change broke the sanitizer buildbots: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/37446

Reverted in b19d87b16f81e7c0a22a0a103c867c1b844eb8bc

******************** TEST 'Clang-Unit :: Frontend/./FrontendTests/PCHPreambleTest.ParseWithBom' FAILED ********************
Note: Google Test filter = PCHPreambleTest.ParseWithBom
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from PCHPreambleTest
[ RUN      ] PCHPreambleTest.ParseWithBom
==39360==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x149ea6f in clang::CompilerInstance::createOutputFile(llvm::StringRef, std::__1::error_code&, bool, bool, llvm::StringRef, llvm::StringRef, bool, bool, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Frontend/CompilerInstance.cpp:730:7
    #1 0x149bbb4 in clang::CompilerInstance::createOutputFile(llvm::StringRef, bool, bool, llvm::StringRef, llvm::StringRef, bool, bool) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Frontend/CompilerInstance.cpp:689:43
    #2 0x15a6240 in clang::GeneratePCHAction::CreateOutputFile(clang::CompilerInstance&, llvm::StringRef, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Frontend/FrontendActions.cpp:141:10
    #3 0x1673eed in (anonymous namespace)::PrecompilePreambleAction::CreateASTConsumer(clang::CompilerInstance&, llvm::StringRef) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Frontend/PrecompiledPreamble.cpp:208:10
    #4 0x158cc44 in clang::FrontendAction::CreateWrappedASTConsumer(clang::CompilerInstance&, llvm::StringRef) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Frontend/FrontendAction.cpp:152:43
    #5 0x1596d7c in clang::FrontendAction::BeginSourceFile(clang::CompilerInstance&, clang::FrontendInputFile const&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Frontend/FrontendAction.cpp:825:9
    #6 0x166b40e in clang::PrecompiledPreamble::Build(clang::CompilerInvocation const&, llvm::MemoryBuffer const*, clang::PreambleBounds, clang::DiagnosticsEngine&, llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>, std::__1::shared_ptr<clang::PCHContainerOperations>, bool, clang::PreambleCallbacks&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Frontend/PrecompiledPreamble.cpp:345:13
    #7 0x1454602 in clang::ASTUnit::getMainBufferWithPrecompiledPreamble(std::__1::shared_ptr<clang::PCHContainerOperations>, clang::CompilerInvocation&, llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>, bool, unsigned int) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Frontend/ASTUnit.cpp:1378:54
    #8 0x145a5a5 in clang::ASTUnit::LoadFromCompilerInvocation(std::__1::shared_ptr<clang::PCHContainerOperations>, unsigned int, llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Frontend/ASTUnit.cpp:1677:9
    #9 0x145bf13 in clang::ASTUnit::LoadFromCompilerInvocation(std::__1::shared_ptr<clang::CompilerInvocation>, std::__1::shared_ptr<clang::PCHContainerOperations>, llvm::IntrusiveRefCntPtr<clang::DiagnosticsEngine>, clang::FileManager*, bool, clang::CaptureDiagsKind, unsigned int, clang::TranslationUnitKind, bool, bool, bool) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Frontend/ASTUnit.cpp:1722:12
    #10 0x5d14d4 in (anonymous namespace)::PCHPreambleTest::ParseAST(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/unittests/Frontend/PCHPreambleTest.cpp:98:36
    #11 0x5da805 in (anonymous namespace)::PCHPreambleTest_ParseWithBom_Test::TestBody() /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/unittests/Frontend/PCHPreambleTest.cpp:230:32
    #12 0x790d1f in HandleExceptionsInMethodIfSupported<testing::Test, void> /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc
    #13 0x790d1f in testing::Test::Run() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2474:5
    #14 0x7937c3 in testing::TestInfo::Run() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2656:11
    #15 0x794d0f in testing::TestCase::Run() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2774:28
    #16 0x7b16d9 in testing::internal::UnitTestImpl::RunAllTests() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4649:43
    #17 0x7b03e7 in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc
    #18 0x7b03e7 in testing::UnitTest::Run() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4257:10
    #19 0x77b994 in RUN_ALL_TESTS /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:2233:46
    #20 0x77b994 in main /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/UnitTestMain/TestMain.cpp:50:10
    #21 0x7f0b7c0732e0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202e0)
    #22 0x4caec9 in _start (/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/tools/clang/unittests/Frontend/FrontendTests+0x4caec9)

SUMMARY: MemorySanitizer: use-of-uninitialized-value /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Frontend/CompilerInstance.cpp:730:7 in clang::CompilerInstance::createOutputFile(llvm::StringRef, std::__1::error_code&, bool, bool, llvm::StringRef, llvm::StringRef, bool, bool, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*)
Exiting

[hans]

Sorry for the breakage, and thanks for reverting! I'll try to follow up and fix this tomorrow.

[zahen]

Sorry about that I'll add a corrected patch

[zahen]

Fixed MemorySanitizer: use-of-uninitialized-value warning

clang/include/clang/Driver/Options.td

 def fno_strict_aliasing : Flag<["-"], "fno-strict-aliasing">, Group<f_Group>,
   Flags<[DriverOption, CoreOption]>;
 def fstruct_path_tbaa : Flag<["-"], "fstruct-path-tbaa">, Group<f_Group>;
 def fno_struct_path_tbaa : Flag<["-"], "fno-struct-path-tbaa">, Group<f_Group>;
 def fno_strict_enums : Flag<["-"], "fno-strict-enums">, Group<f_Group>;
 def fno_strict_vtable_pointers: Flag<["-"], "fno-strict-vtable-pointers">,
   Group<f_Group>;
 def fno_strict_overflow : Flag<["-"], "fno-strict-overflow">, Group<f_Group>;
+def fno_temp_file : Flag<["-"], "fno-temp-file">, Group<f_Group>,
+  Flags<[CC1Option, CoreOption]>, HelpText<"Do not create a temp file during compilation">;
 def fno_threadsafe_statics : Flag<["-"], "fno-threadsafe-statics">, Group<f_Group>,
   Flags<[CC1Option]>, HelpText<"Do not emit code to make initialization of local statics thread safe">;
 def fno_use_cxa_atexit : Flag<["-"], "fno-use-cxa-atexit">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Don't use __cxa_atexit for calling destructors">;
 def fno_register_global_dtors_with_atexit : Flag<["-"], "fno-register-global-dtors-with-atexit">, Group<f_Group>,
   HelpText<"Don't use atexit or __cxa_atexit to register global destructors">;
 def fno_use_init_array : Flag<["-"], "fno-use-init-array">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Don't use .init_array instead of .ctors">;

clang/include/clang/Frontend/FrontendOptions.h

public:
   unsigned BuildingImplicitModule : 1;
 
   /// Whether we should embed all used files into the PCM file.
   unsigned ModulesEmbedAllFiles : 1;
 
   /// Whether timestamps should be written to the produced PCH file.
   unsigned IncludeTimestamps : 1;
 
+  /// Should a temporary file be used during compilation.
+  unsigned UseTemporary : 1;
+
   CodeCompleteOptions CodeCompleteOpts;
 
   /// Specifies the output format of the AST.
   ASTDumpOutputFormat ASTDumpFormat = ADOF_Default;
 
   enum {
     ARCMT_None,
     ARCMT_Check,

clang/lib/Driver/ToolChains/Clang.cpp

#endif
   Args.AddLastArg(CmdArgs, options::OPT_fobjc_sender_dependent_dispatch);
   Args.AddLastArg(CmdArgs, options::OPT_fdiagnostics_print_source_range_info);
   Args.AddLastArg(CmdArgs, options::OPT_fdiagnostics_parseable_fixits);
   Args.AddLastArg(CmdArgs, options::OPT_ftime_report);
   Args.AddLastArg(CmdArgs, options::OPT_ftime_trace);
   Args.AddLastArg(CmdArgs, options::OPT_ftime_trace_granularity_EQ);
   Args.AddLastArg(CmdArgs, options::OPT_ftrapv);
   Args.AddLastArg(CmdArgs, options::OPT_malign_double);
+  Args.AddLastArg(CmdArgs, options::OPT_fno_temp_file);
 
   if (Arg *A = Args.getLastArg(options::OPT_ftrapv_handler_EQ)) {
     CmdArgs.push_back("-ftrapv-handler");
     CmdArgs.push_back(A->getValue());
   }
 
   Args.AddLastArg(CmdArgs, options::OPT_ftrap_function_EQ);
 

clang/lib/Frontend/CompilerInstance.cpp

void CompilerInstance::clearOutputFiles(bool EraseFiles) {
   NonSeekStream.reset();
 }
 
 std::unique_ptr<raw_pwrite_stream>
 CompilerInstance::createDefaultOutputFile(bool Binary, StringRef InFile,
                                           StringRef Extension) {
   return createOutputFile(getFrontendOpts().OutputFile, Binary,
                           /*RemoveFileOnSignal=*/true, InFile, Extension,
-                          /*UseTemporary=*/true);
+                          getFrontendOpts().UseTemporary );
 }
 
 std::unique_ptr<raw_pwrite_stream> CompilerInstance::createNullOutputFile() {
   return std::make_unique<llvm::raw_null_ostream>();
 }
 
 std::unique_ptr<raw_pwrite_stream>
 CompilerInstance::createOutputFile(StringRef OutputPath, bool Binary,

clang/lib/Frontend/CompilerInvocation.cpp

static InputKind ParseFrontendArgs(FrontendOptions &Opts, ArgList &Args,
   for (const auto *A : Args.filtered(OPT_fmodule_file)) {
     StringRef Val = A->getValue();
     if (Val.find('=') == StringRef::npos)
       Opts.ModuleFiles.push_back(Val);
   }
   Opts.ModulesEmbedFiles = Args.getAllArgValues(OPT_fmodules_embed_file_EQ);
   Opts.ModulesEmbedAllFiles = Args.hasArg(OPT_fmodules_embed_all_files);
   Opts.IncludeTimestamps = !Args.hasArg(OPT_fno_pch_timestamp);
+  Opts.UseTemporary = !Args.hasArg(OPT_fno_temp_file); 
 
   Opts.CodeCompleteOpts.IncludeMacros
     = Args.hasArg(OPT_code_completion_macros);
   Opts.CodeCompleteOpts.IncludeCodePatterns
     = Args.hasArg(OPT_code_completion_patterns);
   Opts.CodeCompleteOpts.IncludeGlobals
     = !Args.hasArg(OPT_no_code_completion_globals);
   Opts.CodeCompleteOpts.IncludeNamespaceLevelDecls

clang/lib/Frontend/FrontendActions.cpp

 GeneratePCHAction::CreateOutputFile(CompilerInstance &CI, StringRef InFile,
                                     std::string &OutputFile) {
   // We use createOutputFile here because this is exposed via libclang, and we
   // must disable the RemoveFileOnSignal behavior.
   // We use a temporary to avoid race conditions.
   std::unique_ptr<raw_pwrite_stream> OS =
       CI.createOutputFile(CI.getFrontendOpts().OutputFile, /*Binary=*/true,
                           /*RemoveFileOnSignal=*/false, InFile,
-                          /*Extension=*/"", /*UseTemporary=*/true);
+                          /*Extension=*/"", CI.getFrontendOpts().UseTemporary);
   if (!OS)
     return nullptr;
 
   OutputFile = CI.getFrontendOpts().OutputFile;
   return OS;
 }
 
 bool GeneratePCHAction::shouldEraseOutputFiles() {