- User Since
- Jul 8 2015, 10:26 AM (365 w, 19 h)
May 27 2022
May 25 2022
Apr 27 2022
Mar 24 2022
So long as it doesn't change behavior expected by tapi on Darwin, I think it's OK. I doubt any other platforms are similarly affected.
Sorry for ay delayed replies - I've switched teams at Apple and find it difficult to keep up with llvm reviews.
Dec 6 2021
Looks good to me, thanks.
(And apologies for more delay: I'm not working on llvm much these days, Jessica or Jon (both cc'd) may be able to provide better turnaround time going forward.)
Nov 16 2021
Nov 2 2021
Thanks for the patch. We usually go out of our way to not break llvm-cov workflows. I'd suggest:
- Leaving the old -name-whitelist around for a release, but marked cl::Hidden. Using it should print a deprecation warning.
- Adding a llvm-14 release note (llvm/docs/ReleaseNotes.txt) about the planned deprecation and removal of -name-whitelist. llvm-14 would ship both options.
- Posting on llvm-dev (or a forum) about the change.
- Removing -name-whitelist when llvm-15 branches.
Oct 22 2021
Thanks! Lgtm as well, but with the same nits as Raphael.
Oct 19 2021
Oct 18 2021
Oct 14 2021
Oct 8 2021
Sep 22 2021
Hi @Hiralo, could you share some of the use cases for this feature? What were the pros/cons of any alternatives you considered (e.g. the %m/%c modes)?
Sep 21 2021
Sep 20 2021
Sep 17 2021
Sep 16 2021
- Add missing skipIf(arch=no_match(['arm64', 'arm64e'])) change.
- Limit testing to arm64/arm64e only.
- Set a stop reason for crashing threads only (and not for threads waiting on a syscall to finish).
- Use @jasonmolenda's multi-threaded test case; make sure we only select the crashing thread.
Sep 15 2021
- Add a test (thanks @jasonmolenda!).
FTR, I was able to write a test using process save-core, but even with -s dirty this is generating gigabytes of data and would presumably be super-flaky on the bots.
I also tried -s stack, but for the test program I passed in (just some 2-line .c file that dereferences NULL), process save-core -s stack ... generated an invalid .core with no sections, and lldb couldn't parse it.
Address review feedback:
Sep 14 2021
- Add license header to the .def file.
- (I'm not sure whether/how this change can be tested, any pointers appreciated.)
Sep 1 2021
Yes, thanks! The test from D108929 should cover the lto case.
Aug 31 2021
Aug 30 2021
- Drop end-of-line checks, since this differs between otool new vs. classic.
x86 access shouldn't be necessary. Here's a patch for the otool formatting issue: https://reviews.llvm.org/D108929 (feel free to fold this in to your own change, if you'd like to).
Thanks for tackling this.
Aug 27 2021
Thanks, lgtm with one concern (see inline).
Aug 24 2021
Seconding Keith's comment - please add a test.
@zequanwu I apologize for the delay in reviews. I've skimmed this change, and think it's in the right direction. I will try to respond with an in-depth review by the end of this week, if no one beats me to it.
Aug 6 2021
Aug 5 2021
Aug 3 2021
Aug 2 2021
I'm concerned about introducing another copy of the mmap() logic. The ones for Apple vs. non-Apple have already diverged: e.g. the non-Apple one isn't appending profile data in non-merging mode, and it also doesn't handle unlocking a profile on mmap() failure in the same way. If we add a third copy, we may inadvertently introduce new platform-specific behavior differences. I hesitate to ask that that refactoring happen now, but the sooner the better. Wdyt, Zequan?
Hey @jj10306, thanks for working on this.
Jul 29 2021
@thakis thanks for doing this, lgtm as well.
Hi @jmorse, thanks so much for putting this together. I agree that reverting does seem like the best option right now.
Jul 23 2021
Hey Augusto, thanks for tackling this, I'm just now slowly paging things in.
Jul 20 2021
Drop unneeded braces in switch cases.
Jul 13 2021
Jul 12 2021
Jul 9 2021
Jul 2 2021
Hi, could you say a little bit about the use case(s) for this?
Jul 1 2021
Jun 30 2021
Is this a case where the guidance (as-written) suggests dropping the location instead?
Jun 28 2021
I love the save-session feature. Just checking, has it already been release-noted on llvm.org (and for that matter Xcode)?
Jun 24 2021
FTR, I'm quite happy with the new direction. Thanks!
Jun 23 2021
A one-time exception to the .profraw compatibility policy sounds reasonable to me.
Jun 22 2021
@teemperor thanks for doing this. We addressed a handful of bugs in TAG_call_site emission over multiple clang releases, so I think this is the right fix. It also seems reasonable to make skipUnlessHasCallSiteInfo stricter, but then we'd lose some coverage.
The requirement is that the advancing by sizeof(CovMapFunctionRecordV3) bytes in the record buffer advances to the start of the next record. The address of the buffer doesn't need the same alignment.
Jun 21 2021
llvm has had a long-standing policy of keeping details of the .profraw format private between libprofile and InstrProfReader, opting to only commit to backwards-compat for the indexed .profdata format. I'm concerned that breaking with that practice would send the wrong message and commit llvm to maintaining an extra compat layer for a long time.
Jun 11 2021
I think this is moving in the right direction! However, the way ThreadTrace is specified still raises the same scalability concerns (see inline). The "instruction index" concept doesn't seem sufficient. Decoding needs to be done in two stages: a separate "trace index" concept would help with that. In the first stage, lldb identifies which subset of the trace it needs decoded by specifying start/stop trace indices. This should be fast, as counting the number of available trace bytes/"chunks" is O(1). Nothing needs to be decoded at this point. The second stage starts after a trace subset is decoded, and is where instruction indices are useful.
Jun 10 2021
Thanks for sharing this! Specifically re: the -fprofile-generate v. MIP comparison for code coverage, perhaps it would be more fair to compare against -finstrument-function-entry-bare. The latter only adds one call per function, whereas -fprofile-generate can add instrument a function in more than one place.
@ggeorgakoudis apologies again for the delay here.
Jun 9 2021
Jun 8 2021
I'm quite concerned about the design decision to represent a trace as a vector of TraceInstruction. This bakes considerations that appear to be specific to Facebook's usage of IntelPT way too deep into lldb's APIs, and isn't workable for our use cases.
Thanks, excited to see this work progress!
Jun 2 2021
If llvm stopped appending __profd_foo to llvm.compiler.used, there might not be anything left to prevent the optimizer from discarding it. The current scheme of having the linker ignore N_NO_DEAD_STRIP if S_ATTR_LIVE_SUPPORT is in effect seems like a neat way to solve the issue. Is there a tidy alternative?
While I haven't spotted any issues with this patch, I feel it'd be better for someone with more familiarity with COFF linking to offer a second +1.
Thanks for adding these excellent tests. Lgtm as well.
Thanks Adrian. + 1 from me as well, FTR.