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

[dongjunduo]

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.

[Whitney]

clang/lib/Driver/Driver.cpp

4689

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

4703

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]

clang/lib/Driver/Driver.cpp

4736

Assert size not 0.

[dongjunduo]

[Clang] add -### for debug

[dongjunduo]

Add more debug info

[dongjunduo]

Add more debug info

[jamieschmeiser]

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.

[dongjunduo]

fix stringRef bug

[dongjunduo]

In D131469#3715929, @jamieschmeiser wrote:

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.

[dongjunduo]

format code

[dongjunduo]

Restyle codes

[dongjunduo]

clang/lib/Driver/Driver.cpp

4736

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

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

[dongjunduo]

format cc1_main.cpp

[Whitney]

clang/lib/Driver/Driver.cpp

4736

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() && "...");

[dongjunduo]

Add necessary asserts

[Whitney]

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).

[dongjunduo]

Add assert messages

[jamieschmeiser]

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.

4519

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.

4525

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.

4530

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

4540

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

4551

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

4565

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?

4576

Variables should start with an uppercase letter

[dongjunduo]

Restyle code

[dongjunduo]

clang/lib/Driver/Driver.cpp

4525

The output filename should not be empty.

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

[jamieschmeiser]

clang/lib/Driver/Driver.cpp

4519

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

4525

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

[dongjunduo]

Restyle code

[dongjunduo]

clang/lib/Driver/Driver.cpp

4519

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]

clang/lib/Driver/Driver.cpp

4515

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

4554

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

4591

should we also replace -ftime-trace?

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

5

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

[dongjunduo]

Restyle variables' name and comments

clang/lib/Driver/Driver.cpp

4554

Right

4591

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

[Whitney]

clang/lib/Driver/Driver.cpp

4554

ic, can you please have the comment updated?

4591

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

[dongjunduo]

clang/lib/Driver/Driver.cpp

4554

Done at line:4703

4591

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]

clang/lib/Driver/Driver.cpp

4554

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

4591

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]

clang/lib/Driver/Driver.cpp

4554

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

4591

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

[dongjunduo]

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

[dongjunduo]

Rewrite comments and commit log

[dongjunduo]

@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?

[jamieschmeiser]

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.

[dyung]

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?

[dongjunduo]

In D131469#3768260, @dyung wrote:

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]

In D131469#3768274, @dongjunduo wrote:

In D131469#3768260, @dyung wrote:

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)

[dongjunduo]

In D131469#3768282, @dyung wrote:

In D131469#3768274, @dongjunduo wrote:

In D131469#3768260, @dyung wrote:

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]

In D131469#3768283, @dongjunduo wrote:

In D131469#3768282, @dyung wrote:

In D131469#3768274, @dongjunduo wrote:

In D131469#3768260, @dyung wrote:

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.

[dongjunduo]

In D131469#3768288, @dyung wrote:

In D131469#3768283, @dongjunduo wrote:

In D131469#3768282, @dyung wrote:

In D131469#3768274, @dongjunduo wrote:

In D131469#3768260, @dyung wrote:

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.

[steven_wu]

In D131469#3768308, @dongjunduo wrote:

In D131469#3768288, @dyung wrote:

In D131469#3768283, @dongjunduo wrote:

In D131469#3768282, @dyung wrote:

In D131469#3768274, @dongjunduo wrote:

In D131469#3768260, @dyung wrote:

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]

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]

In D131469#3768352, @steven_wu wrote:

In D131469#3768308, @dongjunduo wrote:

In D131469#3768288, @dyung wrote:

In D131469#3768283, @dongjunduo wrote:

In D131469#3768282, @dyung wrote:

In D131469#3768274, @dongjunduo wrote:

In D131469#3768260, @dyung wrote:

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.

[dongjunduo]

These related commits have been reverted temporarily.

[steven_wu]

In D131469#3768500, @dongjunduo wrote:

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]

Rewrite test by checking output of -###

[dongjunduo]

@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]

fix windows path-check error

[steven_wu]

Thanks

[MaskRay]

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]

clang/lib/Driver/Driver.cpp

4514

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

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

4516

hasArg or hasArgNoClaim

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

4529

delete blank line

4567

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

4589

memory leak. try avoiding raw memory allocation.

4595

https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop

4597

Prefer == to equals

[dongjunduo]

fix related nits

[dongjunduo]

In D131469#3775741, @MaskRay wrote:

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]

@MaskRay ping

[MaskRay]

clang/lib/Driver/Driver.cpp

4516

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.

4517

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

4526

4528

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

[MaskRay]

Sorry, we should compare this approach with D133662

[dongjunduo]

clang/lib/Driver/Driver.cpp

4516

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

[Maetveis]

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

4531–4548

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.

4552–4596

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.

4562–4564

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.

[dongjunduo]

In D131469#3798409, @Maetveis wrote:

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 : )

[Maetveis]

In D131469#3808758, @dongjunduo wrote:

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.

[dongjunduo]

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]

clang/lib/Driver/Driver.cpp

4531–4548

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?

clang/lib/Driver/Driver.cpp

 #include "llvm/Support/MD5.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Program.h"
 #include "llvm/Support/StringSaver.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include <map>
 #include <memory>
 #include <utility>
 #if LLVM_ON_UNIX
 #include <unistd.h> // getpid
 #endif
 
 using namespace clang::driver;
 using namespace clang;
Action *Driver::ConstructPhaseAction(
   }
   case phases::Assemble:
     return C.MakeAction<AssembleJobAction>(std::move(Input), types::TY_Object);
   }
 
   llvm_unreachable("invalid phase in ConstructPhaseAction");
 }
 
+// Infer data storing path of the options `-ftime-trace`, `-ftime-trace=<path>`
+static void inferTimeTracePath(Compilation &C) {
+  bool HasTimeTrace = C.getArgs().hasArg(options::OPT_ftime_trace);
+  bool HasTimeTraceFile = C.getArgs().hasArg(options::OPT_ftime_trace_EQ);
+  // Whether `-ftime-trace` or `-ftime-trace=<path>` are specified
+  if (!HasTimeTrace && !HasTimeTraceFile)
+    return;
+
+  // If `-ftime-trace=<path>` is specified, TracePath is the <path>.
+  // Else if there is a linking job, TracePath is the parent path of .exe,
+  //         then the OutputFile's name may be appended to it.
+  // Else, TracePath is "",
+  //         then the full OutputFile's path may be appended to it.
+  SmallString<128> TracePath("");
+  if (HasTimeTraceFile) {
+    TracePath = SmallString<128>(
+        C.getArgs().getLastArg(options::OPT_ftime_trace_EQ)->getValue());
+  } else {
+    // Get linking executable file's parent path as TracePath's parent path,
+    // default is ".". Filename may be determined and added into TracePath then.
+    //
+    // e.g. executable file's path: /usr/local/a.out
+    //      its parent's path:      /usr/local
+    for (auto &J : C.getJobs()) {
+      if (J.getSource().getKind() == Action::LinkJobClass) {
+        assert(!J.getOutputFilenames().empty() &&
+               "linking output filename is empty");
+        auto OutputFilePath =
+            SmallString<128>(J.getOutputFilenames()[0].c_str());
+        if (llvm::sys::path::has_parent_path(OutputFilePath)) {
+          TracePath = llvm::sys::path::parent_path(OutputFilePath);
+        } else {
+          TracePath = SmallString<128>(".");
+        }
+        break;
+      }
+    }
+  }
+
+  // Add or replace the modified -ftime-trace=<path>` to all clang jobs
+  for (auto &J : C.getJobs()) {
+    if (J.getSource().getKind() == Action::AssembleJobClass ||
+        J.getSource().getKind() == Action::BackendJobClass ||
+        J.getSource().getKind() == Action::CompileJobClass) {
+      SmallString<128> TracePathReal = TracePath;
+      SmallString<128> OutputPath(J.getOutputFilenames()[0].c_str());
+      SmallString<128> arg("-ftime-trace=");
+      if (!HasTimeTraceFile) {
+        if (TracePathReal.empty()) {
+          // /xxx/yyy.o => /xxx/yyy.json
+          llvm::sys::path::replace_extension(OutputPath, "json");
+          arg += OutputPath;
+        } else {
+          // /xxx/yyy.o => /executable_file_parent_path/yyy.json
+          llvm::sys::path::append(TracePathReal,
+                                  llvm::sys::path::filename(OutputPath));
+          llvm::sys::path::replace_extension(TracePathReal, "json");
+          arg += TracePathReal;
+        }
+      } else {
+        // /full_file_path_specified or /path_specified/yyy.json
+        if (llvm::sys::fs::is_directory(TracePathReal))
+          llvm::sys::path::append(TracePathReal,
+                                  llvm::sys::path::filename(OutputPath));
+        llvm::sys::path::replace_extension(TracePathReal, "json");
+        arg += TracePathReal;
+      }
+
+      assert(arg.size() > strlen("-ftime-trace") &&
+             arg.find("-ftime-trace=") == 0 && arg[arg.size() - 1] != '=' &&
+             "invalid `-ftime-trace=<path>`");
+
+      // Replace `-ftime-trace` or `-ftime-trace=<path>` with the modified
+      // `-ftime-trace=<infered_path>`.
+      auto &JArgs = J.getArguments();
+      for (size_t I = 0, S = JArgs.size(); I < S; ++I) {
+        if (StringRef(JArgs[I]).startswith("-ftime-trace=") ||
+            (StringRef(JArgs[I]) == "-ftime-trace" && !HasTimeTraceFile)) {
+          ArgStringList NewArgs(JArgs.begin(), JArgs.begin() + I);
+          NewArgs.push_back(C.getArgs().MakeArgStringRef(arg));
+          NewArgs.append(JArgs.begin() + I + 1, JArgs.end());
+          J.replaceArguments(NewArgs);
+          break;
+        }
+      }
+    }
+  }
+}
+
 void Driver::BuildJobs(Compilation &C) const {
   llvm::PrettyStackTraceString CrashInfo("Building compilation jobs");
 
   Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o);
 
   // It is an error to provide a -o option if we are making multiple output
   // files. There are exceptions:
   //
for (Action *A : C.getActions()) {
     BuildJobsForAction(C, A, &C.getDefaultToolChain(),
                        /*BoundArch*/ StringRef(),
                        /*AtTopLevel*/ true,
                        /*MultipleArchs*/ ArchNames.size() > 1,
                        /*LinkingOutput*/ LinkingOutput, CachedResults,
                        /*TargetDeviceOffloadKind*/ Action::OFK_None);
   }
 
+  // set data storing path of the options `-ftime-trace`, `-ftime-trace=<path>`
+  inferTimeTracePath(C);
+
   // If we have more than one job, then disable integrated-cc1 for now. Do this
   // also when we need to report process execution statistics.
   if (C.getJobs().size() > 1 || CCPrintProcessStats)
     for (auto &J : C.getJobs())
       J.InProcess = false;
 
   if (CCPrintProcessStats) {
     C.setPostCallback([=](const Command &Cmd, int Res) {
       Optional<llvm::sys::ProcessStatistics> ProcStat =
           Cmd.getProcessStatistics();
       if (!ProcStat)
         return;
 
       const char *LinkingOutput = nullptr;
       if (FinalOutput)
         LinkingOutput = FinalOutput->getValue();
       else if (!Cmd.getOutputFilenames().empty())
         LinkingOutput = Cmd.getOutputFilenames().front().c_str();
       else
         LinkingOutput = getDefaultImageName();
C.setPostCallback([=](const Command &Cmd, int Res) {
                             /*Quote*/ true);
         Out << ',';
         llvm::sys::printArg(Out, LinkingOutput, true);
         Out << ',' << ProcStat->TotalTime.count() << ','
             << ProcStat->UserTime.count() << ',' << ProcStat->PeakMemory
             << '\n';
         Out.flush();
         std::error_code EC;
         llvm::raw_fd_ostream OS(CCPrintStatReportFilename, EC,
                                 llvm::sys::fs::OF_Append |
                                     llvm::sys::fs::OF_Text);
         if (EC)
           return;
         auto L = OS.lock();
         if (!L) {
           llvm::errs() << "ERROR: Cannot lock file "
                        << CCPrintStatReportFilename << ": "

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

+// RUN: rm -rf %T/exe && mkdir %T/exe
+// RUN: %clangxx -### -ftime-trace -ftime-trace-granularity=0 -o %T/exe/check-time-trace %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-OPT %s
 // RUN: %clangxx -S -ftime-trace -ftime-trace-granularity=0 -o %T/check-time-trace %s
 // RUN: cat %T/check-time-trace.json \
 // RUN:   | %python -c 'import json, sys; json.dump(json.loads(sys.stdin.read()), sys.stdout, sort_keys=True, indent=2)' \
 // RUN:   | FileCheck %s
 // RUN: %clangxx -S -ftime-trace=%T/new-name.json -ftime-trace-granularity=0 -o %T/check-time-trace %s
 // RUN: cat %T/new-name.json \
 // RUN:   | %python -c 'import json, sys; json.dump(json.loads(sys.stdin.read()), sys.stdout, sort_keys=True, indent=2)' \
 // RUN:   | FileCheck %s
 // RUN: rm -rf %T/output1 && mkdir %T/output1
 // RUN: %clangxx -S -ftime-trace=%T/output1 -ftime-trace-granularity=0 -o %T/check-time-trace %s
 // RUN: cat %T/output1/check-time-trace.json \
 // RUN:   | %python -c 'import json, sys; json.dump(json.loads(sys.stdin.read()), sys.stdout, sort_keys=True, indent=2)' \
 // RUN:   | FileCheck %s
 // RUN: rm -rf %T/output2 && mkdir %T/output2
 // RUN: %clangxx -S -ftime-trace=%T/output2/ -ftime-trace-granularity=0 -o %T/check-time-trace %s
 // RUN: cat %T/output2/check-time-trace.json \
 // RUN:   | %python -c 'import json, sys; json.dump(json.loads(sys.stdin.read()), sys.stdout, sort_keys=True, indent=2)' \
 // RUN:   | FileCheck %s
 
+// CHECK-OPT:  "-ftime-trace={{.*}}{{/|\\\\}}exe{{/|\\\\}}check-time-trace{{.*}}.json"
 // CHECK:      "beginningOfTime": {{[0-9]{16},}}
 // CHECK-NEXT: "traceEvents": [
 // CHECK:      "args":
 // CHECK:      "detail":
 // CHECK:      "dur":
 // CHECK:      "name":
 // CHECK-NEXT: "ph":
 // CHECK-NEXT: "pid":

clang/tools/driver/cc1_main.cpp

int cc1_main(ArrayRef<const char *> Argv, const char *Argv0, void *MainAddr) {
   }
 
   // If any timers were active but haven't been destroyed yet, print their
   // results now.  This happens in -disable-free mode.
   llvm::TimerGroup::printAll(llvm::errs());
   llvm::TimerGroup::clearAll();
 
   if (llvm::timeTraceProfilerEnabled()) {
-    SmallString<128> Path(Clang->getFrontendOpts().OutputFile);
-    llvm::sys::path::replace_extension(Path, "json");
-    if (!Clang->getFrontendOpts().TimeTracePath.empty()) {
-      // replace the suffix to '.json' directly
-      SmallString<128> TracePath(Clang->getFrontendOpts().TimeTracePath);
-      if (llvm::sys::fs::is_directory(TracePath))
-        llvm::sys::path::append(TracePath, llvm::sys::path::filename(Path));
-      Path.assign(TracePath);
-    }
+    assert(!TracePath.empty() && "`-ftime-trace=<path>` is empty");
     if (auto profilerOutput = Clang->createOutputFile(
-            Path.str(), /*Binary=*/false, /*RemoveFileOnSignal=*/false,
+            TracePath.str(), /*Binary=*/false, /*RemoveFileOnSignal=*/false,
             /*useTemporary=*/false)) {
       llvm::timeTraceProfilerWrite(*profilerOutput);
       profilerOutput.reset();
       llvm::timeTraceProfilerCleanup();
       Clang->clearOutputFiles(false);
     }
   }
 
   // Our error handler depends on the Diagnostics object, which we're
   // potentially about to delete. Uninstall the handler now so that any