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

lebedev.ri added inline comments. 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

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

dongjunduo added a comment. > 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

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

Maetveis added a comment. 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

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

dongjunduo added a comment. 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

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

Maetveis requested changes to this revision. Maetveis added a comment. 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

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

dongjunduo added inline comments. Comment at: clang/lib/Driver/Driver.cpp:4516 + 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=` are

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

MaskRay requested changes to this revision. MaskRay added a comment. This revision now requires changes to proceed. Sorry, we should compare this approach with D133662 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

MaskRay accepted this revision. MaskRay added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Driver/Driver.cpp:4516 + bool HasTimeTrace = C.getArgs().hasArg(options::OPT_ftime_trace); + bool HasTimeTraceFile =

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

dongjunduo added a comment. @MaskRay ping 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

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

dongjunduo added a comment. 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

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

dongjunduo updated this revision to Diff 459423. dongjunduo added a comment. fix related nits Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131469/new/ https://reviews.llvm.org/D131469 Files: clang/lib/Driver/Driver.cpp

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

MaskRay added inline comments. Comment at: clang/lib/Driver/Driver.cpp:4514 +// Infer data storing path of the options `-ftime-trace`, `-ftime-trace=` +void InferTimeTracePath(Compilation ) { + bool HasTimeTrace = Use `static`. See

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

MaskRay added a comment. 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= to switch on the TimeProfiler. "We can

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

steven_wu accepted this revision. steven_wu added a comment. Thanks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131469/new/ https://reviews.llvm.org/D131469 ___ cfe-commits mailing list

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

dongjunduo updated this revision to Diff 458440. dongjunduo added a comment. fix windows path-check error Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131469/new/ https://reviews.llvm.org/D131469 Files: clang/lib/Driver/Driver.cpp

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

dongjunduo added a comment. @dyung @steven_wu A newer diff has been submitted just now. Agree with @dyung. The test has been changed into checking the output of "-###". Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131469/new/

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

dongjunduo updated this revision to Diff 458177. dongjunduo added a comment. Rewrite test by checking output of -### Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131469/new/ https://reviews.llvm.org/D131469 Files: clang/lib/Driver/Driver.cpp

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

steven_wu added a comment. 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

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

dongjunduo added a comment. These related commits have been reverted temporarily. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131469/new/ https://reviews.llvm.org/D131469 ___ cfe-commits mailing list

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

dyung added a comment. In D131469#3768352 , @steven_wu wrote: > In D131469#3768308 , @dongjunduo > wrote: > >> In D131469#3768288 , @dyung wrote: >> >>> In

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

thakis added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131469/new/

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

steven_wu added a comment. In D131469#3768308 , @dongjunduo wrote: > In D131469#3768288 , @dyung wrote: > >> In D131469#3768283 , @dongjunduo >> wrote: >> >>> In

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

dongjunduo added a comment. In D131469#3768288 , @dyung wrote: > In D131469#3768283 , @dongjunduo > wrote: > >> In D131469#3768282 , @dyung wrote: >> >>> In

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

dyung added a comment. In D131469#3768283 , @dongjunduo wrote: > In D131469#3768282 , @dyung wrote: > >> In D131469#3768274 , @dongjunduo >> wrote: >> >>> In

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

dongjunduo added a comment. In D131469#3768282 , @dyung wrote: > In D131469#3768274 , @dongjunduo > wrote: > >> In D131469#3768260 , @dyung wrote: >> >>> The test you

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

dyung added a comment. 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

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

dongjunduo added a comment. 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

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

dyung added a comment. 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? Repository: rG LLVM Github

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG38941da066a7: [Clang] change default storing path of `-ftime-trace` (authored by dongjunduo). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131469/new/

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

jamieschmeiser accepted this revision. jamieschmeiser added a comment. 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

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

dongjunduo added a comment. @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

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

dongjunduo updated this revision to Diff 456535. dongjunduo added a comment. Rewrite comments and commit log Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131469/new/ https://reviews.llvm.org/D131469 Files: clang/lib/Driver/Driver.cpp

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

dongjunduo updated this revision to Diff 456533. dongjunduo added a comment. Replace -ftime-trace with -ftime-trace= also Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131469/new/ https://reviews.llvm.org/D131469 Files:

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

dongjunduo marked an inline comment as done. dongjunduo added inline comments. Comment at: clang/lib/Driver/Driver.cpp:4702 + + // Add or replace -ftime-trace` to the correct one to all clang jobs + for (auto : C.getJobs()) { Whitney wrote: > dongjunduo

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

Whitney added inline comments. Comment at: clang/lib/Driver/Driver.cpp:4702 + + // Add or replace -ftime-trace` to the correct one to all clang jobs + for (auto : C.getJobs()) { dongjunduo wrote: > Whitney wrote: > > dongjunduo wrote: > > > Whitney wrote: > >

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

dongjunduo marked an inline comment as done. dongjunduo added inline comments. Comment at: clang/lib/Driver/Driver.cpp:4702 + + // Add or replace -ftime-trace` to the correct one to all clang jobs + for (auto : C.getJobs()) { Whitney wrote: > dongjunduo

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

Whitney added inline comments. Comment at: clang/lib/Driver/Driver.cpp:4702 + + // Add or replace -ftime-trace` to the correct one to all clang jobs + for (auto : C.getJobs()) { dongjunduo wrote: > Whitney wrote: > > Do you mean Add or replace the modified

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

dongjunduo added inline comments. Comment at: clang/lib/Driver/Driver.cpp:4702 + + // Add or replace -ftime-trace` to the correct one to all clang jobs + for (auto : C.getJobs()) { Whitney wrote: > Do you mean Add or replace the modified `-ftime-trace=` to

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

dongjunduo updated this revision to Diff 456282. dongjunduo marked 2 inline comments as done. dongjunduo added a comment. Restyle variables' name and comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131469/new/

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

Whitney added inline comments. Comment at: clang/lib/Driver/Driver.cpp:4663 +void InferTimeTracePath(Compilation ) { + bool TimeTrace = C.getArgs().getLastArg(options::OPT_ftime_trace) != nullptr; + bool TimeTraceFile = Usually bool variable has name that

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

dongjunduo marked 2 inline comments as done. dongjunduo added inline comments. Comment at: clang/lib/Driver/Driver.cpp:4667 + // Whether `-ftime-trace` or `-ftime-trace=` are specified + if (!TimeTrace && !TimeTraceFile) return; + jamieschmeiser wrote: > The

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

dongjunduo updated this revision to Diff 454501. dongjunduo added a comment. Restyle code Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131469/new/ https://reviews.llvm.org/D131469 Files: clang/lib/Driver/Driver.cpp

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

jamieschmeiser added inline comments. Comment at: clang/lib/Driver/Driver.cpp:4667 + // Whether `-ftime-trace` or `-ftime-trace=` are specified + if (!TimeTrace && !TimeTraceFile) return; + The return should be on the next line. Did you run this through

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

dongjunduo marked 7 inline comments as done. dongjunduo added inline comments. Comment at: clang/lib/Driver/Driver.cpp:4674 +for (auto : C.getJobs()) { + if (J.getSource().getKind() == Action::LinkJobClass && + !J.getOutputFilenames().empty()) {

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

dongjunduo updated this revision to Diff 454439. dongjunduo added a comment. Restyle code Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131469/new/ https://reviews.llvm.org/D131469 Files: clang/lib/Driver/Driver.cpp

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

jamieschmeiser added a comment. Good progress. Comment at: clang/lib/Driver/Driver.cpp:97 #include "llvm/Support/raw_ostream.h" +#include #include This include isn't necessary. There are asserts already in the file so this is transitively included.

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

dongjunduo updated this revision to Diff 452058. dongjunduo added a comment. Add assert messages Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131469/new/ https://reviews.llvm.org/D131469 Files: clang/lib/Driver/Driver.cpp

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

Whitney added inline comments. Comment at: clang/tools/driver/cc1_main.cpp:260 +SmallString<128> TracePath(Clang->getFrontendOpts().TimeTracePath); +assert(!TracePath.empty()); if (auto profilerOutput = Clang->createOutputFile( According to llvm

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

dongjunduo updated this revision to Diff 452054. dongjunduo added a comment. Add necessary asserts Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131469/new/ https://reviews.llvm.org/D131469 Files: clang/lib/Driver/Driver.cpp

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

Whitney added inline comments. Comment at: clang/lib/Driver/Driver.cpp:4786 +const std::string::size_type size = arg.size(); +char *buffer = new char[size + 1]; +memcpy(buffer, arg.c_str(), size + 1); dongjunduo wrote: > Whitney wrote: >

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

dongjunduo updated this revision to Diff 451874. dongjunduo added a comment. format cc1_main.cpp Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131469/new/ https://reviews.llvm.org/D131469 Files: clang/lib/Driver/Driver.cpp

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

dongjunduo marked 3 inline comments as done. dongjunduo added inline comments. Comment at: clang/lib/Driver/Driver.cpp:4786 +const std::string::size_type size = arg.size(); +char *buffer = new char[size + 1]; +memcpy(buffer, arg.c_str(), size + 1);

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

dongjunduo updated this revision to Diff 451865. dongjunduo added a comment. Restyle codes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131469/new/ https://reviews.llvm.org/D131469 Files: clang/lib/Driver/Driver.cpp

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

dongjunduo updated this revision to Diff 451857. dongjunduo added a comment. format code Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131469/new/ https://reviews.llvm.org/D131469 Files: clang/lib/Driver/Driver.cpp

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

dongjunduo added a comment. 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

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

dongjunduo updated this revision to Diff 451838. dongjunduo added a comment. fix stringRef bug Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131469/new/ https://reviews.llvm.org/D131469 Files: clang/lib/Driver/Driver.cpp

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

jamieschmeiser added a comment. 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.

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

dongjunduo updated this revision to Diff 451804. dongjunduo added a comment. Add more debug info Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131469/new/ https://reviews.llvm.org/D131469 Files: clang/lib/Driver/Driver.cpp

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

dongjunduo updated this revision to Diff 451796. dongjunduo added a comment. Add more debug info Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131469/new/ https://reviews.llvm.org/D131469 Files: clang/lib/Driver/Driver.cpp

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

dongjunduo updated this revision to Diff 451785. dongjunduo added a comment. [Clang] add -### for debug Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131469/new/ https://reviews.llvm.org/D131469 Files: clang/lib/Driver/Driver.cpp

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

Whitney added inline comments. Comment at: clang/lib/Driver/Driver.cpp:4786 +const std::string::size_type size = arg.size(); +char *buffer = new char[size + 1]; +memcpy(buffer, arg.c_str(), size + 1); Assert size not 0. Repository: rG

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

Whitney added inline comments. Comment at: clang/lib/Driver/Driver.cpp:4739 + // set data storing path of the options `-ftime-trace`, `-ftime-trace=` + bool TimeTrace = C.getArgs().getLastArg(options::OPT_ftime_trace) != nullptr; It would be cleaner if all

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

dongjunduo created this revision. Herald added a project: All. dongjunduo requested review of this revision. Herald added subscribers: cfe-commits, MaskRay. Herald added a project: clang. Change the default storing path of `-ftime-trace`. We can use `-ftime-trace` or `-ftime-trace=` to switch on