Page MenuHomePhabricator

vsk (Vedant Kumar)
User

Projects

User does not belong to any projects.

User Details

User Since
Jul 8 2015, 10:26 AM (365 w, 19 h)

Recent Activity

May 27 2022

vsk accepted D126548: [InstrProf] Stop exporting lprofDirMode.
May 27 2022, 10:05 AM · Restricted Project, Restricted Project, Restricted Project

May 25 2022

vsk accepted D126345: [Clang][CoverageMapping] Fix switch case counter compile time explosion.
May 25 2022, 4:28 PM · Restricted Project, Restricted Project, Restricted Project

Apr 27 2022

vsk added inline comments to D124567: [dsymutil] Fix memory issue in the BinaryHolder.
Apr 27 2022, 4:31 PM · Restricted Project, Restricted Project
vsk added inline comments to D124567: [dsymutil] Fix memory issue in the BinaryHolder.
Apr 27 2022, 4:30 PM · Restricted Project, Restricted Project
vsk accepted D124567: [dsymutil] Fix memory issue in the BinaryHolder.

Thanks, lgtm.

Apr 27 2022, 4:29 PM · Restricted Project, Restricted Project
vsk added inline comments to D124567: [dsymutil] Fix memory issue in the BinaryHolder.
Apr 27 2022, 4:28 PM · Restricted Project, Restricted Project
vsk added inline comments to D124567: [dsymutil] Fix memory issue in the BinaryHolder.
Apr 27 2022, 4:00 PM · Restricted Project, Restricted Project

Mar 24 2022

vsk added a comment to D122336: [InstrProfiling] No runtime hook for unused funcs.

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.

Mar 24 2022, 5:03 PM · Restricted Project, Restricted Project, Restricted Project
vsk updated subscribers of D122336: [InstrProfiling] No runtime hook for unused funcs.

Sorry for ay delayed replies - I've switched teams at Apple and find it difficult to keep up with llvm reviews.

Mar 24 2022, 12:09 PM · Restricted Project, Restricted Project, Restricted Project

Dec 6 2021

vsk added a comment to D96854: [CodeExtractor] Enable partial aggregate arguments.

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

Dec 6 2021, 2:58 PM · Restricted Project

Nov 16 2021

vsk removed a member for debug-info: vsk.
Nov 16 2021, 10:38 AM

Nov 2 2021

vsk added a comment to D112816: [LLVM][llvm-cov] Inclusive language: rename option -name-whitelist to -name-allowlist.

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.
Nov 2 2021, 12:44 PM · Restricted Project

Oct 22 2021

vsk added a comment to D112325: [lldb] Pass the target triple to the compiler when determining the DWARF version.

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

Oct 22 2021, 9:53 AM · Unknown Object (Project)

Oct 19 2021

vsk committed rG5e004b03f72a: [lldb/test] Update test/API/functionalities/load_lazy to macOS 12 (authored by vsk).
[lldb/test] Update test/API/functionalities/load_lazy to macOS 12
Oct 19 2021, 1:25 PM
vsk closed D112034: [lldb/test] Update test/API/functionalities/load_lazy to macOS 12.
Oct 19 2021, 1:25 PM · Unknown Object (Project)

Oct 18 2021

vsk requested review of D112034: [lldb/test] Update test/API/functionalities/load_lazy to macOS 12.
Oct 18 2021, 2:41 PM · Unknown Object (Project)

Oct 14 2021

vsk accepted D111793: [Driver][Darwin] Use T reference instead of getToolChain().getTriple()..

Thanks, lgtm.

Oct 14 2021, 10:41 AM · Restricted Project

Oct 8 2021

vsk committed rG96f937746e19: [ADT] Mark IntervalMap::overlaps const (authored by vsk).
[ADT] Mark IntervalMap::overlaps const
Oct 8 2021, 3:22 PM

Sep 22 2021

vsk added a comment to D110232: SBCC RFE: Support SBCC for Shared Library.

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 22 2021, 10:09 AM · Restricted Project
vsk updated subscribers of D96854: [CodeExtractor] Enable partial aggregate arguments.
Sep 22 2021, 10:05 AM · Restricted Project

Sep 21 2021

vsk added a comment to D110013: [lldb][crashlog] Avoid specifying arch for image when a UUID is present.

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?

Sep 21 2021, 3:38 PM · Unknown Object (Project)

Sep 20 2021

vsk committed rGe31b2d7d7be9: [lldb][crashlog] Avoid specifying arch for image when a UUID is present (authored by vsk).
[lldb][crashlog] Avoid specifying arch for image when a UUID is present
Sep 20 2021, 10:24 AM
vsk closed D110013: [lldb][crashlog] Avoid specifying arch for image when a UUID is present.
Sep 20 2021, 10:24 AM · Unknown Object (Project)

Sep 17 2021

vsk committed rG3b14d80ad4af: [MachCore] Report arm64 thread exception state (authored by vsk).
[MachCore] Report arm64 thread exception state
Sep 17 2021, 4:46 PM
vsk requested review of D110013: [lldb][crashlog] Avoid specifying arch for image when a UUID is present.
Sep 17 2021, 4:34 PM · Unknown Object (Project)

Sep 16 2021

vsk added a reverting change for rG7eb67748f9d7: [MachCore] Report arm64 thread exception state: rG79e48f3c7c8c: Revert "[MachCore] Report arm64 thread exception state".
Sep 16 2021, 1:44 PM
vsk committed rG79e48f3c7c8c: Revert "[MachCore] Report arm64 thread exception state" (authored by vsk).
Revert "[MachCore] Report arm64 thread exception state"
Sep 16 2021, 1:44 PM
vsk added a reverting change for D109795: [MachCore] Report arm64 thread exception state: rG79e48f3c7c8c: Revert "[MachCore] Report arm64 thread exception state".
Sep 16 2021, 1:44 PM · Unknown Object (Project)
vsk committed rG7eb67748f9d7: [MachCore] Report arm64 thread exception state (authored by vsk).
[MachCore] Report arm64 thread exception state
Sep 16 2021, 1:35 PM
vsk closed D109795: [MachCore] Report arm64 thread exception state.
Sep 16 2021, 1:35 PM · Unknown Object (Project)
vsk updated the diff for D109795: [MachCore] Report arm64 thread exception state.
  • Add missing skipIf(arch=no_match(['arm64', 'arm64e'])) change.
Sep 16 2021, 11:25 AM · Unknown Object (Project)
vsk updated the diff for D109795: [MachCore] Report arm64 thread exception state.
  • 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 16 2021, 11:21 AM · Unknown Object (Project)

Sep 15 2021

vsk added inline comments to D109795: [MachCore] Report arm64 thread exception state.
Sep 15 2021, 5:00 PM · Unknown Object (Project)
vsk updated the diff for D109795: [MachCore] Report arm64 thread exception state.
Sep 15 2021, 4:40 PM · Unknown Object (Project)
vsk added a comment to D109795: [MachCore] Report arm64 thread exception state.

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.

Sep 15 2021, 3:46 PM · Unknown Object (Project)
vsk added inline comments to D109795: [MachCore] Report arm64 thread exception state.
Sep 15 2021, 2:49 PM · Unknown Object (Project)
vsk updated the diff for D109795: [MachCore] Report arm64 thread exception state.

Address review feedback:

Sep 15 2021, 2:48 PM · Unknown Object (Project)

Sep 14 2021

vsk updated the diff for D109795: [MachCore] Report arm64 thread exception state.
  • Add license header to the .def file.
  • (I'm not sure whether/how this change can be tested, any pointers appreciated.)
Sep 14 2021, 5:03 PM · Unknown Object (Project)
vsk requested review of D109795: [MachCore] Report arm64 thread exception state.
Sep 14 2021, 5:01 PM · Unknown Object (Project)
vsk committed rG66902a32c838: [StopInfoMachException] Summarize arm64e BLRAx/LDRAx auth failures (authored by vsk).
[StopInfoMachException] Summarize arm64e BLRAx/LDRAx auth failures
Sep 14 2021, 2:05 PM
vsk closed D102428: [StopInfoMachException] Summarize arm64e BLRAx/LDRAx auth failures.
Sep 14 2021, 2:04 PM · Unknown Object (Project)

Sep 1 2021

vsk accepted D105445: [InstrProfiling] Use llvm.compiler.used if applicable for Mach-O.

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

Sep 1 2021, 11:48 AM · Restricted Project, Restricted Project

Aug 31 2021

vsk committed rG6c439a38172b: [profile] Specify "-V" to otool to get expected test output (authored by vsk).
[profile] Specify "-V" to otool to get expected test output
Aug 31 2021, 10:50 AM
vsk closed D108929: [profile] Specify "-V" to otool to get expected test output.
Aug 31 2021, 10:49 AM · Restricted Project

Aug 30 2021

vsk updated the diff for D108929: [profile] Specify "-V" to otool to get expected test output.
Aug 30 2021, 11:31 AM · Restricted Project
vsk updated the diff for D108929: [profile] Specify "-V" to otool to get expected test output.
  • Drop end-of-line checks, since this differs between otool new vs. classic.
Aug 30 2021, 11:29 AM · Restricted Project
vsk added a comment to D105445: [InstrProfiling] Use llvm.compiler.used if applicable for Mach-O.

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

Aug 30 2021, 11:27 AM · Restricted Project, Restricted Project
vsk requested review of D108929: [profile] Specify "-V" to otool to get expected test output.
Aug 30 2021, 11:26 AM · Restricted Project
vsk added a comment to D105445: [InstrProfiling] Use llvm.compiler.used if applicable for Mach-O.

Thanks for tackling this.

Aug 30 2021, 10:10 AM · Restricted Project, Restricted Project

Aug 27 2021

vsk accepted D108242: [Profile] Support __llvm_profile_set_file_object in continuous mode..

Thanks, lgtm with one concern (see inline).

Aug 27 2021, 11:44 AM · Restricted Project

Aug 24 2021

vsk added inline comments to D108242: [Profile] Support __llvm_profile_set_file_object in continuous mode..
Aug 24 2021, 2:06 PM · Restricted Project
vsk added a comment to D105139: [llvm-cov] Allow multiple remaps in --path-equivalence.

Seconding Keith's comment - please add a test.

Aug 24 2021, 1:56 PM · Restricted Project
vsk added a comment to D108242: [Profile] Support __llvm_profile_set_file_object in continuous mode..

@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 24 2021, 1:45 PM · Restricted Project
vsk added inline comments to D108127: [cmake] By default do not instrument compiler-rt if LLVM_BUILD_INSTRUMENTED_COVERAGE is ON.
Aug 24 2021, 1:42 PM · Restricted Project

Aug 6 2021

vsk added inline comments to D107591: [Profile][NFC] Clean up initializeProfileForContinuousMode.
Aug 6 2021, 1:26 PM · Restricted Project
vsk accepted D107591: [Profile][NFC] Clean up initializeProfileForContinuousMode.

Thanks, lgtm.

Aug 6 2021, 11:20 AM · Restricted Project

Aug 5 2021

vsk added a comment to D107591: [Profile][NFC] Clean up initializeProfileForContinuousMode.

Thanks!

Aug 5 2021, 3:03 PM · Restricted Project

Aug 3 2021

vsk added a comment to D107203: [Profile] Support __llvm_profile_set_file_object in continuous mode..

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?

Aug 3 2021, 12:05 PM · Restricted Project

Aug 2 2021

vsk committed rG3b0a9e7b392a: [profile] Move assertIsZero to InstrProfilingUtil.c (authored by vsk).
[profile] Move assertIsZero to InstrProfilingUtil.c
Aug 2 2021, 3:25 PM
vsk added a comment to D107203: [Profile] Support __llvm_profile_set_file_object in continuous mode..

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?

Aug 2 2021, 3:24 PM · Restricted Project
vsk added a comment to D105741: [trace] Introduce Hierarchical Trace Representation (HTR) and add `thread trace export ctf` command for Intel PT trace visualization.

Hey @jj10306, thanks for working on this.

Aug 2 2021, 12:09 PM · Unknown Object (Project)

Jul 29 2021

vsk added a comment to D106733: [clang/darwin] Pass libclang_rt.profile last on linker command.

@thakis thanks for doing this, lgtm as well.

Jul 29 2021, 10:56 AM · Restricted Project
vsk accepted D107076: [DWARF] Revert sharing subprograms across CUs.

Hi @jmorse, thanks so much for putting this together. I agree that reverting does seem like the best option right now.

Jul 29 2021, 10:55 AM · Restricted Project

Jul 23 2021

vsk added a comment to D106584: [lldb] Assert file cache and live memory are equal on debug builds.

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

Jul 23 2021, 2:45 PM · Unknown Object (Project)

Jul 20 2021

vsk accepted D106314: [Utils] Add -compilation-dir flag to prepare-code-coverage-artifact.py.
Jul 20 2021, 10:15 AM · Restricted Project
vsk updated the diff for D102428: [StopInfoMachException] Summarize arm64e BLRAx/LDRAx auth failures.

Drop unneeded braces in switch cases.

Jul 20 2021, 9:43 AM · Unknown Object (Project)

Jul 13 2021

vsk committed rG5105a77035d0: [docs/llvm-cov] Document -compilation-dir (authored by vsk).
[docs/llvm-cov] Document -compilation-dir
Jul 13 2021, 1:10 PM
vsk closed D105826: [docs/llvm-cov] Document -compilation-dir.
Jul 13 2021, 1:10 PM · Restricted Project
vsk updated the diff for D105826: [docs/llvm-cov] Document -compilation-dir.

Include -ffile-compilation-dir.

Jul 13 2021, 9:44 AM · Restricted Project

Jul 12 2021

vsk added a comment to D105139: [llvm-cov] Allow multiple remaps in --path-equivalence.

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.

Jul 12 2021, 10:08 AM · Restricted Project
vsk requested review of D105826: [docs/llvm-cov] Document -compilation-dir.
Jul 12 2021, 9:41 AM · Restricted Project

Jul 9 2021

vsk updated subscribers of D70350: [DWARF] Allow cross-CU references of subprogram definitions.

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

Jul 9 2021, 2:28 PM · Restricted Project

Jul 2 2021

vsk added a comment to D105139: [llvm-cov] Allow multiple remaps in --path-equivalence.

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

Jul 2 2021, 9:23 AM · Restricted Project

Jul 1 2021

vsk added a comment to D105162: Avoid misleading line table when Localizer sinks an instruction to another basic block.

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?

Jul 1 2021, 5:46 PM · Restricted Project

Jun 30 2021

vsk added a comment to D105162: Avoid misleading line table when Localizer sinks an instruction to another basic block.

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

Jun 30 2021, 9:25 AM · Restricted Project

Jun 28 2021

vsk added a comment to D105030: [lldb/Interpreter] Add setting to set session transcript save directory.

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

Jun 28 2021, 10:18 AM · Unknown Object (Project)

Jun 24 2021

vsk added a comment to D104422: [trace] Add a TraceCursor class.

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

Jun 24 2021, 4:53 PM · Unknown Object (Project)
vsk added inline comments to D104839: [profile] Decommit memory after counter relocation.
Jun 24 2021, 3:25 PM · Restricted Project
vsk accepted D104745: [llvm-cov] Enforce alignment of function records.

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:

Jun 24 2021, 12:55 PM · Restricted Project

Jun 23 2021

vsk added a comment to D104556: [InstrProfiling] Make CountersPtr in __profd_ relative.

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

Jun 23 2021, 9:53 AM · Restricted Project, Restricted Project, Restricted Project
vsk added a comment to D104745: [llvm-cov] Enforce alignment of function records.

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.

Jun 23 2021, 9:32 AM · Restricted Project

Jun 22 2021

vsk accepted rG98e2b1a8dd8f: [lldb] Adjust Clang version requirements for tail_call_frames tests.
Jun 22 2021, 4:14 PM
vsk added a comment to rG98e2b1a8dd8f: [lldb] Adjust Clang version requirements for tail_call_frames tests.

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

Jun 22 2021, 4:14 PM
vsk added a comment to D104745: [llvm-cov] Enforce alignment of function records.

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 22 2021, 4:07 PM · Restricted Project

Jun 21 2021

vsk added a comment to D104556: [InstrProfiling] Make CountersPtr in __profd_ relative.

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.

Jun 21 2021, 4:25 PM · Restricted Project, Restricted Project, Restricted Project
vsk added a comment to D104556: [InstrProfiling] Make CountersPtr in __profd_ relative.

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.

Jun 21 2021, 4:16 PM · Restricted Project, Restricted Project, Restricted Project
vsk added a comment to D104556: [InstrProfiling] Make CountersPtr in __profd_ relative.

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 21 2021, 3:33 PM · Restricted Project, Restricted Project, Restricted Project

Jun 11 2021

vsk added a comment to D103588: [trace] Create a top level ThreadTrace class.

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 11 2021, 2:30 PM · Unknown Object (Project)

Jun 10 2021

vsk added a comment to D104060: Machine IR Profile.

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.

Jun 10 2021, 4:42 PM · Restricted Project
vsk added a comment to D25456: [InstrProf] Add support for dead_strip+live_support functionality.
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.

Jun 10 2021, 2:25 PM · Restricted Project
vsk added a comment to D96854: [CodeExtractor] Enable partial aggregate arguments.

@ggeorgakoudis apologies again for the delay here.

Jun 10 2021, 2:21 PM · Restricted Project

Jun 9 2021

vsk added a comment to D103588: [trace] Create a top level ThreadTrace class.

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.

Jun 9 2021, 3:34 PM · Unknown Object (Project)

Jun 8 2021

vsk added a comment to D103588: [trace] Create a top level ThreadTrace class.

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.

Jun 8 2021, 5:07 PM · Unknown Object (Project)
vsk added a comment to D103588: [trace] Create a top level ThreadTrace class.

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.

Jun 8 2021, 2:52 PM · Unknown Object (Project)
vsk requested changes to D103588: [trace] Create a top level ThreadTrace class.

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.

Jun 8 2021, 11:09 AM · Unknown Object (Project)
vsk requested changes to D103500: [trace][intel-pt] Create basic SB API.

Thanks, excited to see this work progress!

Jun 8 2021, 10:38 AM · Unknown Object (Project)

Jun 2 2021

vsk added a comment to D25456: [InstrProf] Add support for dead_strip+live_support functionality.

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?

Jun 2 2021, 2:24 PM · Restricted Project
vsk added a comment to D103372: [InstrProfiling] If no value profiling, make data variable private and (for Windows) use one comdat.

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.

Jun 2 2021, 2:11 PM · Restricted Project, Restricted Project, Restricted Project
vsk added a comment to D103355: [InstrProfiling] Delete linkage/visibility toggling for Windows.

Thanks for adding these excellent tests. Lgtm as well.

Jun 2 2021, 2:01 PM · Restricted Project, Restricted Project
vsk added a comment to D103220: Support stripping indirectly referenced DILocations from !llvm.loop metadata in stripDebugInfo().

Thanks Adrian. + 1 from me as well, FTR.

Jun 2 2021, 7:53 AM · Restricted Project