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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
