Sorry, it looks like this was accidentally committed without a Phab tag in: https://github.com/llvm/llvm-project/commit/4831f4b7bdeb22e405248c45b3ea607a6b28b991

Thanks, lgtm.

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.

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

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.

Thanks! Lgtm as well, but with the same nits as Raphael.

Thanks, lgtm.

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

In D110013#3013733, @aprantl wrote:

Without having dug into this deeper, do you think not specifying the arch could cause any problems with universal binaries? I guess not, because we still have the UUID, and the UUID is per arch?

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

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

• Add license header to the .def file.
• (I'm not sure whether/how this change can be tested, any pointers appreciated.)

Yes, thanks! The test from D108929 should cover the lto case.

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

Thanks, lgtm with one concern (see inline).

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.

Thanks, lgtm.

Thanks!

In D107203#2923010, @zequanwu wrote:

In D107203#2921037, @vsk wrote:

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?

Do you mean something like the TODO at https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/profile/InstrProfilingFile.c#L429, moving those platform specific code to InstrProfilingPlatform* files?

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.

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

Hey Augusto, thanks for tackling this, I'm just now slowly paging things in.

Drop unneeded braces in switch cases.

Include -ffile-compilation-dir.

In D105139#2856196, @tcamin wrote:

Hi,

Thanks for taking a look at this.

Our application is split into different modules which get built as separate libraries. These libraries are prebuilt and are shared among our devs as a build cache to reduce compilation times. These libraries do not necessarily share a single map between build path and source code location and therefore we’re unable to get coverage data out all of them.

Our requirement is being able to extract coverage data when running integration tests.

In D70350#2867905, @kyulee wrote:

@vsk As commented in https://bugs.llvm.org/show_bug.cgi?id=48790, this seems to impact us when building a large app with LTO even in XCode.
I don't have a cutdown but it showed up frequently blocking us stack traces with symbolication. Have we considered fixing this or backing out this change?

Hi, could you say a little bit about the use case(s) for this?

In D105162#2854369, @aprantl wrote:

In D105162#2850394, @vsk wrote:

Is this a case where the guidance (as-written) suggests dropping the location instead?

Yes. And I wonder if we should update the guidance. Literally dropping the location would result in the last instruction's location being carried forward. We don't want the inserted instruction to keep its location from the other basic block, but associating it with the previous instruction's location would also be wrong since we are inserting it on behalf of the next instruction (the insertion point). Maybe this isn't so much sinking an instruction but expanding an existing instruction?

Is this a case where the guidance (as-written) suggests dropping the location instead?

I love the save-session feature. Just checking, has it already been release-noted on llvm.org (and for that matter Xcode)?

FTR, I'm quite happy with the new direction. Thanks!

In D104745#2837127, @serge-sans-paille wrote:

I'm completely happy to see refactors/cleanups in this area. I just wanted to make sure we're all on the same page about whether or not there's an alignment bug here. E.g. if there is such a bug that can lead to a crash, it would make sense to add a regression test.

Actually, the test already exist: current code crashes the test suite on llvm-conv/*branch* tests on RHEL7 on i686 machine. Let me try to explain the bug in a different way:

A one-time exception to the .profraw compatibility policy sounds reasonable to me.

In D104745#2835126, @serge-sans-paille wrote:

In D104745#2834796, @vsk wrote:

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.

The function advanceByOne(...) starts with the following assert: assert(isAddrAligned(Align(8), this) && "Function record not aligned"); so I tend to think each record needs to be aligned, including the first.

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

In D104556#2831849, @davidxl wrote:

In D104556#2831837, @vsk wrote:

In D104556#2831833, @davidxl wrote:

In D104556#2831800, @nickdesaulniers wrote:

In D104556#2831757, @vsk wrote:

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.

Why/how does the Linux PGO patch "care" about the .profraw format?

Because the Linux kernel cannot statically link against compiler-rt; we MUST emulate the interfaces provided by compiler-rt within the Linux kernel's runtime.

One way to solve this is to use a matching version of llvm-profdata tool and export indexed profile data only.

I don't have much experience with the Linux build system, so apologies if this is naive, but why is it exactly that it can't statically link libprofile? This is something we do for the Darwin kernel (there's a stripped-down runtime target called libclang_rt.cc_kext_x86_64_osx.a the kernel links).

Does it use buffer API? It does not support value profiling.

There may be other reasons.

In D104556#2831833, @davidxl wrote:

In D104556#2831800, @nickdesaulniers wrote:

In D104556#2831757, @vsk wrote:

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.

Why/how does the Linux PGO patch "care" about the .profraw format?

Because the Linux kernel cannot statically link against compiler-rt; we MUST emulate the interfaces provided by compiler-rt within the Linux kernel's runtime.

One way to solve this is to use a matching version of llvm-profdata tool and export indexed profile data only.

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.

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.

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.

In D25456#2796732, @MaskRay wrote:

In D25456#2794689, @vsk wrote:

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?

Currently __profd_foo is in llvm.used instead of llvm.compiler.used on Mach-O.

D103372 My thought (but I don't have a device to test) is that Mach-O can use appendToCompilerUsed(*M, CompilerUsedVars); as well.

@ggeorgakoudis apologies again for the delay here.

In D103588#2806894, @wallace wrote:

I've been thinking about what you said and I'm having second thoughts on my implementation. I'll share more context:

• I want to work in the short term on reverse debugging and reconstruction of stack traces, for that i'll need to know the instruction type of each instruction in the trace, which will be used as part of some heuristics to identify calls and returns between functions.
• A future application that we plan to work on is adding profiling information to the instructions
• Right now the intel-pt plugin is storing the decoded instructions in a vector, which works for small traces but wont' for gigantic traces. I imagine that I'll end up removing that vector and make the TraverseInstruction API decode instructions in place keeping one instruction in memory at a time within the intel pt plugin for a given traversal. For that I'll need accessors that can provide information of the current Instruction. As there could be two or more concurrent traversals happening at the same time (I'm trying to be generic here), it might make sense to create an abstract class TraceInstruction that can be extended by each plug-in and implement its getters.

I'm thinking about something like this

class TraceInstruction {
  virtual lldb::addr_t GetLoadAddress() = 0;
  virtual TraceInstructionType() GetInstructionType() = 0;
  virtual uint64_t GetTimestamp() = 0;
  ... anything that can appear in the future 
};

and have no members, leaving to each plug-in the decision of which of those methods to implement and how.

What do you think of this? I think this incorporates your feedback.

In D103588#2806611, @wallace wrote:

This approach - of prototyping trace analyses on a vector<TraceInstruction> representation and then rewriting them later when scaling issues arise - doesn't sound good to me. Even for something simple, like finding a backtrace, the window of instructions needed for analysis can easily grow to a point where the full vector<TraceInstruction> can't be maintained in memory.

It's worth noticing that the vector<TraceInstruction> is internal to the Intel PT plugin. It's not exposed at the Trace.h interface level, so any consumers can't make any assumptions on how the data is stored. We will improve the intel pt plugin itself as we make progress on this, but we are ensuring that the interfaces are correct for the future.

In D103588#2806512, @wallace wrote:

Right now we haven't implemented lazy decoding; we are decoding the entire trace. But that's just because so far we have been working with small traces. As we progress in this work, we'll start working with larger traces and we'll have to do implement the lazy decoding, for which the TraverseInstructions API will come handy.

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!

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.