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

[Mészáros Gergely via Phabricator via cfe-commits]

Maetveis requested changes to this revision.
Maetveis added a comment.

As discussed with @jamieschmeiser on D133662 <https://reviews.llvm.org/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.



================
Comment at: clang/lib/Driver/Driver.cpp:4531-4548
+    // 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) {
----------------
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.


================
Comment at: clang/lib/Driver/Driver.cpp:4552-4596
+  // 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());
----------------
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 `Tool`s (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`.


================
Comment at: clang/lib/Driver/Driver.cpp:4562-4564
+          // /xxx/yyy.o => /xxx/yyy.json
+          llvm::sys::path::replace_extension(OutputPath, "json");
+          arg += OutputPath;
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131469/new/

https://reviews.llvm.org/D131469