Page MenuHomePhabricator

Fix for Bug 50033 - -fno-temp-file is not respected when creating a pch in clang 12
ClosedPublic

Authored by zahen on May 3 2021, 11:30 AM.

Details

Summary

When creating a PCH file the use of a temp file will be dictated by the presence or absence of the -fno-temp-file flag. Creating a module file will always use a temp file via the new ForceUseTemporary flag.

This design was worked out over email with @dexonsmith.

Once accepted I'll need someone to commit this change on my behalf.

Diff Detail

Unit TestsFailed

TimeTest
24,010 msx64 debian > libFuzzer.libFuzzer::entropic-scale-per-exec-time.test
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/mnt/disks/ssd0/agent/llvm-project/compiler-rt/lib/fuzzer -m64 /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/fuzzer/EntropicScalePerExecTimeTest.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/entropic-scale-per-exec-time.test.tmp-EntropicScalePerExecTimeTest

Event Timeline

zahen requested review of this revision.May 3 2021, 11:30 AM
zahen created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2021, 11:30 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Yep, this fixes a regression (https://bugs.llvm.org/show_bug.cgi?id=50033) caused by ad7aaa475e5e632242b07380ec47d2f35d077209, where I somehow missed that modules and PCHs had different actual behaviour around -fno-temp-file despite sharing the same comments.

The code LGTM. Ideally there would be a test, but it's a bit hard to add a test for this, given that temp files aren't currently observable. Can you think of something? (Maybe it's worth adding a remark for debugging purposes when a temp file is used? With or without the filename in it? Although that could be awkward to thread through LLVM when the output backends get virtualized... What do others think?)

If no one has ideas in the next few days, I'll "accept" and commit for you. (Feel free to ping me; there's a good chance I'll lose track of this otherwise; usual ping rate is 1x/week, but happy for you to ping me sooner on this...)

zahen added a comment.May 3 2021, 11:51 AM

Thanks for taking a look right away Duncan! The same concern about a lack of tests was raised when I first added -fno-temp-file and I'm afraid inspiration hasn't struck me in the meantime.

zahen added a comment.May 10 2021, 7:51 AM

If no one has ideas in the next few days, I'll "accept" and commit for you. (Feel free to ping me; there's a good chance I'll lose track of this otherwise; usual ping rate is 1x/week, but happy for you to ping me sooner on this...)

@dexonsmith Ping as requested. Unfortunately I haven't come up with any way to get this tested in a cross-platform manner.

zahen added a comment.May 24 2021, 9:21 AM

@dexonsmith anything else required before this can land?

dexonsmith accepted this revision.Jun 1 2021, 12:52 PM

LGTM; thanks for you patience.

I've been a bit paralyzed, thinking about whether we could/should add a remark, something like -Rtemp-file or -Routput-file, which could be triggered when opening an output file. That would be a path toward testing this. On the other hand, I'm afraid maintaining the remark could restrict options in the future for output backends. For now let's just go with what you have.

This revision is now accepted and ready to land.Jun 1 2021, 12:52 PM

@zahen , what author information should I use for the Git commit?

zahen added a comment.Jun 1 2021, 12:55 PM

@zahen , what author information should I use for the Git commit?

Zachary Henkel, zahen@microsoft.com (GitHub username ZacharyHenkel)

Thanks Duncan!

dexonsmith closed this revision.Jun 18 2021, 10:59 AM

I pushed this as 05d0f1a8ea012a6b7b8ea65893ec4121106444b5 last night, but I just realized I forgot to include the link to the review :(. Closing manually.