This is an archive of the discontinued LLVM Phabricator instance.

[Clang] change default storing path of `-ftime-trace`
Needs RevisionPublic

Authored by dongjunduo on Aug 9 2022, 12:49 AM.

Details

Summary

-ftime-trace or -ftime-trace=<path> enable the TimeProfiler.

When Clang does compile and link actions in one command and -ftime-trace is used, the created time-trace json files currently goes to a temporary directory, e.g.

$ clang++ -ftime-trace -o main.out /demo/main.cpp
$ ls .
main.out
$ ls /tmp/
main-[random-string].json

Obviously the time-trace files cannot be easily found, so that the json file location is undesired.

This patch change the default storing behavior of -ftime-trace.

That is, if the compiling job contains the linking action, the executable file' s directory may be seem as the main work directory.
Thus the time trace files would be stored in the same directory of linking result.

By this approach, the user can easily get the time-trace files in the main work directory. The improved demo results:

$ clang++ -ftime-trace -o main.out /demo/main.cpp
$ ls .
main.out   main-[random-string].json

In addition, the main codes of time-trace files' path inference have been refactored.

  • The <path> of -ftime-trace=<path> is infered in clang driver
  • After that, -ftime-trace=<path> can be added into clang's options

By this approach, the dirty work of path processing and judging can be implemented in driver layer, so that the clang may focus on its main work.

Diff Detail

Event Timeline

dongjunduo created this revision.Aug 9 2022, 12:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2022, 12:49 AM
dongjunduo requested review of this revision.Aug 9 2022, 12:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2022, 12:49 AM
Whitney added inline comments.Aug 9 2022, 6:10 AM
clang/lib/Driver/Driver.cpp
4825

It would be cleaner if all these logic can be moved to a separate function with a description.

4839

Do you expect link only show up once? If so, you can add a break here.

clang/tools/driver/cc1_main.cpp
261

Should we assert that TimeTracePath is not empty?

Whitney added inline comments.Aug 9 2022, 6:22 AM
clang/lib/Driver/Driver.cpp
4872

Assert size not 0.

inclyc added a subscriber: inclyc.Aug 9 2022, 11:17 AM

[Clang] add -### for debug

Add more debug info

Add more debug info

You should not have debugging information in code that is up for review. If this is debugging information that you plan to leave in for future purposes (which I doubt is the case here), you need to protect it so that it isn't active unless some option is set. For example, see LLVM_DEBUG code that lives in various opt routines.

fix stringRef bug

You should not have debugging information in code that is up for review. If this is debugging information that you plan to leave in for future purposes (which I doubt is the case here), you need to protect it so that it isn't active unless some option is set. For example, see LLVM_DEBUG code that lives in various opt routines.

Got it. I want to use this unnecessary print to find why some tests passed in local but failed in CI. They will be removed soon.

format code

Restyle codes

dongjunduo marked 3 inline comments as done.Aug 11 2022, 8:10 AM
dongjunduo added inline comments.
clang/lib/Driver/Driver.cpp
4872

The size of arg maynot be 0 because of its basic initialization in L:4690

std::string arg = std::string("-ftime-trace=");

format cc1_main.cpp

Whitney added inline comments.Aug 11 2022, 8:53 AM
clang/lib/Driver/Driver.cpp
4872

How about assert that it starts with -ftime-trace= and not ends with =.

clang/tools/driver/cc1_main.cpp
264

As it is unexpected, it should be an assert:
assert(!TracePath.empty() && "...");

Add necessary asserts

dongjunduo marked 2 inline comments as done.Aug 11 2022, 7:18 PM
Whitney added inline comments.Aug 11 2022, 7:19 PM
clang/tools/driver/cc1_main.cpp
260

According to llvm coding standards:
To further assist with debugging, make sure to put some kind of error message in the assertion statement (which is printed if the assertion is tripped).

Add assert messages

dongjunduo marked an inline comment as done.Aug 11 2022, 7:32 PM

Good progress.

clang/lib/Driver/Driver.cpp
97

This include isn't necessary. There are asserts already in the file so this is transitively included.

4668

I think the quick return style is generally preferred. In this case, test the negative and return. It is easier to follow and limits indenting.

4674

Can you have a link job without an output filename? If not, then just have an assert for !empty. Again, the negative test and continue might be easier to understand.

4679

you create the same small string twice. better to create a temporary.

4689

Again, test negative with continue
Do you also want CompileJobClass?

4700

You are changing TracePath, which will then be altered when you loop back. I think you need to use a copy of TracePath.

4714

Can you compute the full path to the directory before the loop and then just copy it and add the name in the loop for each file?

4725

Variables should start with an uppercase letter

Restyle code

dongjunduo marked 7 inline comments as done.Aug 22 2022, 3:18 AM
dongjunduo added inline comments.
clang/lib/Driver/Driver.cpp
4674

The output filename should not be empty.

If the "-o" is not specified, the output filename may be "a.out".

jamieschmeiser added inline comments.Aug 22 2022, 7:03 AM
clang/lib/Driver/Driver.cpp
4668

The return should be on the next line. Did you run this through clang-format? Is it okay with this on the same line?

4674

Yes. If the output filename should not be empty, then assert that rather than testing it.

Restyle code

dongjunduo marked 2 inline comments as done.Aug 22 2022, 7:39 AM
dongjunduo added inline comments.
clang/lib/Driver/Driver.cpp
4668

Yeah. I always use 'clang-format -i xxx.cpp' to check the code style, but some old codes may be restyled to a new format which I haven't modified.

So I restore the code but forget to check the new added code then.

Whitney added inline comments.Aug 22 2022, 10:06 AM
clang/lib/Driver/Driver.cpp
4664

Usually bool variable has name that starts with Has or something similar, e.g., HasTimeTraceFile.

4703

Do you mean Add or replace the modified -ftime-trace=<path> to all clang jobs?

4740

should we also replace -ftime-trace?

clang/test/Driver/check-time-trace.cpp
5

what may be between check-time-trace and .json?

dongjunduo marked 2 inline comments as done.Aug 29 2022, 1:39 AM

Restyle variables' name and comments

clang/lib/Driver/Driver.cpp
4703

Right

4740

The work before here is to infer the correct path to store the time-trace file.

After that, the <path> in -ftime-trace=<path> should be replaced by the infered correct path.

We do not need to replace -ftime-trace then.

clang/test/Driver/check-time-trace.cpp
5

If we use -ftime-trace but no -ftime-trace=<path> to compile the source like "check-time-trace.cpp" to a executable file check-time-trace, the check-time-trace.cpp should be compiled to check-time-trace-[random-string].o, then linked to the check-time-trace by the linker. This random string is introduced by clang's own default logic.

The -ftime-trace records the time cost details of compilng source file to the object file (.cpp -> .o). If the time-trace file name isn't be specified, its default name is [object file's name].json, which is corresponding the object file.

Demo:

Cmd: clang++ -ftime-trace -ftime-trace-granularity=0 -o check-time-trace check-time-trace.cpp
Output: check-time-trace, check-time-trace-a40601.json

dongjunduo edited the summary of this revision. (Show Details)Aug 29 2022, 7:25 AM
Whitney added inline comments.Aug 29 2022, 8:28 AM
clang/lib/Driver/Driver.cpp
4703

ic, can you please have the comment updated?

4740

What happens when -ftime-trace is given by the user? Do you have both -ftime-trace=<path> and -ftime-trace as arguments?

dongjunduo marked an inline comment as done.Aug 29 2022, 7:15 PM
dongjunduo added inline comments.
clang/lib/Driver/Driver.cpp
4703

Done at line:4703

4740

It doesn't matter that either "-ftime-trace" or "-ftime-trace=<path>" or both of them are given by the user.

The TimeProfiler is switched on when either "-ftime-trace" and "-ftime-trace=<path>" is specified. Then,

  • If "-ftime-trace=<path>" is specified, the driver use <path>.
  • If only "-ftime-trace" is specified, the <path> can be infered, then a new option "-ftime-trace=<infered_path>" may be added to the clang.
Whitney added inline comments.Aug 29 2022, 7:25 PM
clang/lib/Driver/Driver.cpp
4703

Let me clarify...can you please add = in between -ftime-trace and <path> for the comment on line 4703?

4740

In the case of only "-ftime-trace" is specified, isn't it better to only pass "-ftime-trace=<infered_path>" to the compile step instead of both "-ftime-trace=<infered_path>" and "-ftime-trace"?

dongjunduo marked an inline comment as done.Aug 29 2022, 7:30 PM
dongjunduo added inline comments.
clang/lib/Driver/Driver.cpp
4703

Sorry, I may have made a spelling mistake : ( hh

4740

You are right... May be it's more suitable... I wll remove the "-ftime-trace" in this situration then.

Replace -ftime-trace with -ftime-trace=<infered_path> also

Rewrite comments and commit log

dongjunduo marked 2 inline comments as done.Aug 29 2022, 8:43 PM

@jamieschmeiser @Whitney

For now, the time-trace file's name is corresponding to the output file's name ([demo].o => [demo].json).

The only fly in the ointment is that when the user hasn't given the object file's name by "-o",
the object file may be stored in /tmp and its name may be append a random string.
The main logic of storing it to a temporary file is here: link.
The random string is created by createUniquePath.

It has guaranteed the object files' name is unique in the /tmp.
If the time-trace file's name follows its corresponding object file's name, it may also be unique.
But if the time-trace file's name follows its corresponding source file's name, it will cause a naming conflict.

Think about a demo common case:

$ clang++ -ftime-trace dir1/source.cpp dir2/source.cpp main.cpp

The object files'name of dir1/source.cpp and dir2/source.cpp must be different, but...

  1. If the time-trace file's name follows the object file, "source-[random-string-1].json" and "source-[random-string-2].json" may be created independently.
  2. If the time-trace file's name follows the source file, "source.json" will be created twice and the first one may be overwritten.

I prefer the first one, which is implemented now.
While this method adds an annoying random string, it is guaranteed not to cause data overwrites in normal use.

So do we need to change it to the second approach?

Whitney accepted this revision.Sep 2 2022, 7:25 AM
This revision is now accepted and ready to land.Sep 2 2022, 7:25 AM
jamieschmeiser accepted this revision.Sep 2 2022, 12:27 PM

If the source is specified with a partial path, I would expect the .json file to be in the same directory as the source, so dir1/f.C and dir2/f.C would result in dir1/f.json and dir2/f.json. But this can be a later improvement. Let's get this landed.

This revision was automatically updated to reflect the committed changes.
dyung added a subscriber: dyung.Sep 2 2022, 7:08 PM

The test you added is failing on the PS4 linux bot because the PS4 platform requires an external linker that isn't present. Is linking necessary for your test? Or can -S or even -c work instead to accomplish what you are trying to test?

The test you added is failing on the PS4 linux bot because the PS4 platform requires an external linker that isn't present. Is linking necessary for your test? Or can -S or even -c work instead to accomplish what you are trying to test?

Yeah the new test case is designed to test the compiling jobs with a linking stage.

Are there any options or measures to avoid this test running on the PS4?

dyung added a comment.Sep 2 2022, 7:23 PM

The test you added is failing on the PS4 linux bot because the PS4 platform requires an external linker that isn't present. Is linking necessary for your test? Or can -S or even -c work instead to accomplish what you are trying to test?

Yeah the new test case is designed to test the compiling jobs with a linking stage.

Are there any options or measures to avoid this test running on the PS4?

You could mark it as XFAIL: ps4

Your change also seems to have possibly the same issue when run on our PS5 Windows bot:
https://lab.llvm.org/buildbot/#/builders/216/builds/9260

$ ":" "RUN: at line 2"
$ "z:\test\build\bin\clang.exe" "--driver-mode=g++" "-ftime-trace" "-ftime-trace-granularity=0" "-o" "Z:\test\build\tools\clang\test\Driver\Output/exe/check-time-trace" "Z:\test\llvm-project\clang\test\Driver\check-time-trace.cpp"
# command stderr:
clang: error: unable to execute command: program not executable
clang: error: linker command failed with exit code 1 (use -v to see invocation)

The test you added is failing on the PS4 linux bot because the PS4 platform requires an external linker that isn't present. Is linking necessary for your test? Or can -S or even -c work instead to accomplish what you are trying to test?

Yeah the new test case is designed to test the compiling jobs with a linking stage.

Are there any options or measures to avoid this test running on the PS4?

You could mark it as XFAIL: ps4

Your change also seems to have possibly the same issue when run on our PS5 Windows bot:
https://lab.llvm.org/buildbot/#/builders/216/builds/9260

$ ":" "RUN: at line 2"
$ "z:\test\build\bin\clang.exe" "--driver-mode=g++" "-ftime-trace" "-ftime-trace-granularity=0" "-o" "Z:\test\build\tools\clang\test\Driver\Output/exe/check-time-trace" "Z:\test\llvm-project\clang\test\Driver\check-time-trace.cpp"
# command stderr:
clang: error: unable to execute command: program not executable
clang: error: linker command failed with exit code 1 (use -v to see invocation)

How about "UNSUPPORTED: ps4, ps5"

dyung added a comment.Sep 2 2022, 7:40 PM

The test you added is failing on the PS4 linux bot because the PS4 platform requires an external linker that isn't present. Is linking necessary for your test? Or can -S or even -c work instead to accomplish what you are trying to test?

Yeah the new test case is designed to test the compiling jobs with a linking stage.

Are there any options or measures to avoid this test running on the PS4?

You could mark it as XFAIL: ps4

Your change also seems to have possibly the same issue when run on our PS5 Windows bot:
https://lab.llvm.org/buildbot/#/builders/216/builds/9260

$ ":" "RUN: at line 2"
$ "z:\test\build\bin\clang.exe" "--driver-mode=g++" "-ftime-trace" "-ftime-trace-granularity=0" "-o" "Z:\test\build\tools\clang\test\Driver\Output/exe/check-time-trace" "Z:\test\llvm-project\clang\test\Driver\check-time-trace.cpp"
# command stderr:
clang: error: unable to execute command: program not executable
clang: error: linker command failed with exit code 1 (use -v to see invocation)

How about "UNSUPPORTED: ps4, ps5"

That would likely work I think.

The test you added is failing on the PS4 linux bot because the PS4 platform requires an external linker that isn't present. Is linking necessary for your test? Or can -S or even -c work instead to accomplish what you are trying to test?

Yeah the new test case is designed to test the compiling jobs with a linking stage.

Are there any options or measures to avoid this test running on the PS4?

You could mark it as XFAIL: ps4

Your change also seems to have possibly the same issue when run on our PS5 Windows bot:
https://lab.llvm.org/buildbot/#/builders/216/builds/9260

$ ":" "RUN: at line 2"
$ "z:\test\build\bin\clang.exe" "--driver-mode=g++" "-ftime-trace" "-ftime-trace-granularity=0" "-o" "Z:\test\build\tools\clang\test\Driver\Output/exe/check-time-trace" "Z:\test\llvm-project\clang\test\Driver\check-time-trace.cpp"
# command stderr:
clang: error: unable to execute command: program not executable
clang: error: linker command failed with exit code 1 (use -v to see invocation)

How about "UNSUPPORTED: ps4, ps5"

That would likely work I think.

Done with the commit 39221ad.

The test you added is failing on the PS4 linux bot because the PS4 platform requires an external linker that isn't present. Is linking necessary for your test? Or can -S or even -c work instead to accomplish what you are trying to test?

Yeah the new test case is designed to test the compiling jobs with a linking stage.

Are there any options or measures to avoid this test running on the PS4?

You could mark it as XFAIL: ps4

Your change also seems to have possibly the same issue when run on our PS5 Windows bot:
https://lab.llvm.org/buildbot/#/builders/216/builds/9260

$ ":" "RUN: at line 2"
$ "z:\test\build\bin\clang.exe" "--driver-mode=g++" "-ftime-trace" "-ftime-trace-granularity=0" "-o" "Z:\test\build\tools\clang\test\Driver\Output/exe/check-time-trace" "Z:\test\llvm-project\clang\test\Driver\check-time-trace.cpp"
# command stderr:
clang: error: unable to execute command: program not executable
clang: error: linker command failed with exit code 1 (use -v to see invocation)

How about "UNSUPPORTED: ps4, ps5"

That would likely work I think.

Done with the commit 39221ad.

I don't like how this done. This generally won't work for Darwin platform as well since you don't know where SDK is. Whether you have a linker or not has nothing to do with the host platform, you need to check for if the linker is available.

This is also a regression in test coverage since all the supported tests in lines after you added are no longer executed on the platform you excludes.

thakis added a subscriber: thakis.Sep 2 2022, 10:55 PM

This breaks tests on Mac and windows, see eg http://45.33.8.238/macm1/43791/step_7.txt

Please take a look and revert for now if it takes a while to fix.

dyung added a comment.Sep 2 2022, 11:43 PM

The test you added is failing on the PS4 linux bot because the PS4 platform requires an external linker that isn't present. Is linking necessary for your test? Or can -S or even -c work instead to accomplish what you are trying to test?

Yeah the new test case is designed to test the compiling jobs with a linking stage.

Are there any options or measures to avoid this test running on the PS4?

You could mark it as XFAIL: ps4

Your change also seems to have possibly the same issue when run on our PS5 Windows bot:
https://lab.llvm.org/buildbot/#/builders/216/builds/9260

$ ":" "RUN: at line 2"
$ "z:\test\build\bin\clang.exe" "--driver-mode=g++" "-ftime-trace" "-ftime-trace-granularity=0" "-o" "Z:\test\build\tools\clang\test\Driver\Output/exe/check-time-trace" "Z:\test\llvm-project\clang\test\Driver\check-time-trace.cpp"
# command stderr:
clang: error: unable to execute command: program not executable
clang: error: linker command failed with exit code 1 (use -v to see invocation)

How about "UNSUPPORTED: ps4, ps5"

That would likely work I think.

Done with the commit 39221ad.

I don't like how this done. This generally won't work for Darwin platform as well since you don't know where SDK is. Whether you have a linker or not has nothing to do with the host platform, you need to check for if the linker is available.

This is also a regression in test coverage since all the supported tests in lines after you added are no longer executed on the platform you excludes.

Thinking it over, I agree with this.

Historically, tests in clang/llvm do not do linking because it can cause a ton of problems (like it seems to be doing). Can you achieve what you are trying to test by maybe examining the -### output? Or if linking is really required, maybe the test-suite would be a better place for this test.

These related commits have been reverted temporarily.

These related commits have been reverted temporarily.

Thanks. Another way to do this is that as you don't really care what linker does in this test case, you just need to fake a linker with a shell script then let the clang driver to invoke that script as a linker.

dongjunduo reopened this revision.Sep 6 2022, 6:40 AM
This revision is now accepted and ready to land.Sep 6 2022, 6:40 AM
dongjunduo updated this revision to Diff 458177.Sep 6 2022, 8:08 AM

Rewrite test by checking output of -###

@dyung @steven_wu A newer diff has been submitted just now.

Agree with @dyung. The test has been changed into checking the output of "-###".

dongjunduo updated this revision to Diff 458440.Sep 7 2022, 6:47 AM

fix windows path-check error

steven_wu accepted this revision.Sep 7 2022, 9:30 AM

Thanks

MaskRay requested changes to this revision.Sep 7 2022, 2:41 PM
This revision now requires changes to proceed.Sep 7 2022, 2:41 PM

I appreciate the detailed summary. It has sufficient information but I think it can be rephased to be conciser.
I request changes for the verbosity and the possibly functionality issue.

We can use -ftime-trace or -ftime-trace=<path> to switch on the TimeProfiler.

"We can use" can be omitted => "-ftime-trace or -ftime-trace=<path> enables ..."

The source files should be compiled into object files (.o) by clang, and then linked into executable file by the linker.

This is basic knowledge and can be removed.

"-ftime-trace or -ftime-trace=<path> enables ..."

Where to store for now?

If we want a detailed description, the whole paragraph is really something which should go to https://clang.llvm.org/docs/UsersManual.html
Then, this patch summary (commit message) can just refer to it.


I'd delete everything and keep just

-ftime-trace and -ftime-trace=<path> enable the TimeProfiler.
When Clang does compile and link actions in one command and -ftime-trace is used, the JSON file currently goes to a temporary directory, e.g.

$ clang++ -ftime-trace -o main.out /demo/main.cpp
$ ls .
main.out
$ ls /tmp/
main-[random-string].json

The JSON file location is undesired. This patch changes the location based on the specified ...


What if an input file and the output is in directories, e.g.

% myclang -ftime-trace a/a.c -o b/a; ls *.json
a-a7cffa.json

Does the patch achieve what it intends to do?

MaskRay added inline comments.Sep 7 2022, 3:06 PM
clang/lib/Driver/Driver.cpp
4663

Use static. See https://llvm.org/docs/CodingStandards.html#anonymous-namespaces

Use lowerCase style for new function names. Ignore existing inconsistency in Clang.

4665

hasArg or hasArgNoClaim

The former claims the option so that it won't trigger a -Wunused-command-line-argument diagnostic.

4678

delete blank line

4716

This means arg probably should be a SmallString as well to prevent unneeded std::string construction.

4738

memory leak. try avoiding raw memory allocation.

4744
4746

Prefer == to equals

fix related nits

dongjunduo edited the summary of this revision. (Show Details)Sep 12 2022, 5:15 AM

I appreciate the detailed summary. It has sufficient information but I think it can be rephased to be conciser.
I request changes for the verbosity and the possibly functionality issue.

We can use -ftime-trace or -ftime-trace=<path> to switch on the TimeProfiler.

"We can use" can be omitted => "-ftime-trace or -ftime-trace=<path> enables ..."

The source files should be compiled into object files (.o) by clang, and then linked into executable file by the linker.

This is basic knowledge and can be removed.

"-ftime-trace or -ftime-trace=<path> enables ..."

Where to store for now?

If we want a detailed description, the whole paragraph is really something which should go to https://clang.llvm.org/docs/UsersManual.html
Then, this patch summary (commit message) can just refer to it.


I'd delete everything and keep just

-ftime-trace and -ftime-trace=<path> enable the TimeProfiler.
When Clang does compile and link actions in one command and -ftime-trace is used, the JSON file currently goes to a temporary directory, e.g.

$ clang++ -ftime-trace -o main.out /demo/main.cpp
$ ls .
main.out
$ ls /tmp/
main-[random-string].json

The JSON file location is undesired. This patch changes the location based on the specified ...


What if an input file and the output is in directories, e.g.

% myclang -ftime-trace a/a.c -o b/a; ls *.json
a-a7cffa.json

Does the patch achieve what it intends to do?

Nice idea. I have simplified the related description. : )

dongjunduo marked 7 inline comments as done.Sep 12 2022, 5:19 AM
MaskRay accepted this revision.Sep 13 2022, 10:18 PM
MaskRay added inline comments.
clang/lib/Driver/Driver.cpp
4665

Change this variable to use getLastArg(options::OPT_ftime_trace_EQ) instead.

The convention is to use getLastArg if both hasArg and getLastArg are needed.

4666

Delete the comment. Don't add comment which just repeats what simply code does.

4675
4677

Delete SmallString<128> => TracePath = ...

This revision is now accepted and ready to land.Sep 13 2022, 10:18 PM
MaskRay requested changes to this revision.Sep 13 2022, 10:23 PM

Sorry, we should compare this approach with D133662

This revision now requires changes to proceed.Sep 13 2022, 10:23 PM
dongjunduo added inline comments.Sep 13 2022, 11:47 PM
clang/lib/Driver/Driver.cpp
4665

Emmm, do you means HasTimeTrace and HasTimeTraceFile's initialized approach shoud be reverted into the last version?

Maetveis requested changes to this revision.Sep 18 2022, 12:20 PM

As discussed with @jamieschmeiser on D133662, I have left suggestions regarding the approach I took for handling -o and passing the option to the jobs.

I'm really happy to see this area getting attention, and I'm sorry for the confusion I must have caused.

clang/lib/Driver/Driver.cpp
4680–4697

Instead of looking for linking jobs, to determine the folder to store the traces, you could use the output location from -o <output> (if specified).
This is already available in Driver::BuildJobs as FinalOutput (see on line 4605), it will be nullptr if no -o option was given.

4701–4745

Instead of rewriting job command lines after they have been created, I think it would be cleaner split handling of -ftime-trace to two parts:

  • Infer the folder where the traces are stored
  • For each job that supports time trace, communicate the path of where it should be stored to it, so it can be used during building the command line.

For the first I think this function is already fine together with the other suggestions, but it must be called before job command lines are created i.e. before BuildJobsForAction is called.

The inferred folder will have to be stored to be used for building the final json locations, In my changes I have stored it as a member in Compilation. Ultimately it would be used in BuildJobsForActionNoCache called by BuildJobsForAction so it could also be passed through as a function argument.

Then Tools (initially this should be just Clang) which support -ftime-trace, can signal it using a method added to Tool (Tools already provide all kinds of information about themselves through their interface.)

If such a tool is selected during building a job for an action (BuildJobsForActionNoCache after Tool has been selected), then the location should be prepared and communicated to it. In my patch I have again used a member in Compilation for this as it's already part of the interface, but this feels at least a little bit hacky, so if you or the other reviewers have better ideas. then go for it.

Based on the discussion on D133662, we want to keep this patch to the basic functionality to be extended later, so It should be fine to select the file names based on the output names. In BuildJobsForActionNoCache that is available as Result, you could use it with the logic you already have in the current second part of inferTimeTracePath.

4711–4713

I think it would be better to fallback to the current working directory instead of the location of the object file.

  • It is consistent with e.g. if no -o is given then the executable will be stored to a.out in the current working directory.
  • It is a more discoverable location for users than the object file, which is often in /temp, when multiple jobs are involved.

As discussed with @jamieschmeiser on D133662, I have left suggestions regarding the approach I took for handling -o and passing the option to the jobs.

I'm really happy to see this area getting attention, and I'm sorry for the confusion I must have caused.

I'm sorry that I didn't see the comments until now because of some personal work... Thank you for your kindness and suppot, I will improve this part of work according to your idea in D133662 : )

I'm sorry that I didn't see the comments until now because of some personal work... Thank you for your kindness and suppot, I will improve this part of work according to your idea in D133662 : )

Hey @dongjunduo,
Are you still interested in finishing this? If not I was thinking that I would pick it up, so I can work on the follow ups.

Hey @dongjunduo,
Are you still interested in finishing this? If not I was thinking that I would pick it up, so I can work on the follow ups.

@Maetveis Sorry for delaying the issue due to some personal trifles...

This part of the implementation has been preliminarily completed. I will submit the new patch in a day or two.

lebedev.ri added inline comments.
clang/lib/Driver/Driver.cpp
4680–4697

Looks like this was not addressed.
I would suspect, not doing so will break The most common use case
of separately compiling sources and then linking them together, does it not?