Page MenuHomePhabricator

Add an -fno-temp-file flag for compilation
ClosedPublic

Authored by zahen on Nov 22 2019, 1:06 PM.

Details

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.

Diff Detail

Event Timeline

zahen created this revision.Nov 22 2019, 1:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 22 2019, 1:06 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rnk added a comment.Dec 4 2019, 3:03 PM

+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 added a comment.Dec 5 2019, 4:53 AM

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.

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 this revision to Diff 232441.Dec 5 2019, 1:22 PM

Updated with review feedback and formatting fixes

zahen updated this revision to Diff 232442.Dec 5 2019, 1:27 PM

Fixed patch

zahen added a comment.Dec 5 2019, 1:56 PM

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 added a comment.Dec 6 2019, 2:12 AM

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 added a comment.Dec 8 2019, 8:35 PM

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.


hans accepted this revision.Dec 10 2019, 4:09 AM

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.

This revision is now accepted and ready to land.Dec 10 2019, 4:09 AM
zahen added a comment.Dec 10 2019, 6:45 AM

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

zahen added a comment.Dec 17 2019, 8:08 PM

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

hans added a comment.Dec 18 2019, 6:08 AM

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.

This revision was automatically updated to reflect the committed changes.
hctim added a subscriber: hctim.Dec 18 2019, 9:06 AM

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 added a comment.Dec 18 2019, 9:23 AM

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

zahen reopened this revision.Dec 18 2019, 9:32 AM

Sorry about that I'll add a corrected patch

This revision is now accepted and ready to land.Dec 18 2019, 9:32 AM
zahen updated this revision to Diff 234563.Dec 18 2019, 9:41 AM

Fixed MemorySanitizer: use-of-uninitialized-value warning

This revision was automatically updated to reflect the committed changes.