Page MenuHomePhabricator

vsk (Vedant Kumar)
User

Projects

User Details

User Since
Jul 8 2015, 10:26 AM (219 w, 4 d)

Recent Activity

Fri, Sep 20

vsk accepted D67398: [DebugInfo] LiveDebugValues: Move DBG_VALUE creation into VarLoc class.

Really nice!

Fri, Sep 20, 1:25 PM · Restricted Project
vsk accepted D67393: [DebugInfo] LiveDebugValues: Defer all DBG_VALUE creation during analysis.

First, DBG_VALUEs aren't necessarily source-level assignments *before* LiveDebugValues either, they update the SSA value that a (fragment) of a source-level variable can be found in, but that SSA value could have been created by the compiler and has not necessarily any relation to a source-level assignment (think about salvageDebugInfo, for example).

(Unfortunately I'm not known for operating the English language effectively). My meaning in the wording was about the placement of a dbg.value/DBG_VALUE within a block, ignoring its operand. AFAIUI, for any LLVM-IR instruction i in a function, and a (fragment of) variable, to determine the variables location at i one has to do an entire dominance-frontier analysis to work out which dbg.value/DBG_VALUE dominates i (potentially none of them). This method of recording the _position_ where variable locations _change_, closely matches the original source program: if you had five assignments to a variable in a program, you'd have five dbg.values, regardless of their operands.

The fact that the DBG_VALUE has no effect outside of the current basic block just falls out of DbgEntityHistoryCalculator not doing a LiveDebugVariable-style data flow analysis, but IMO that isn't a change in semantics, it would be *legal* for it to perform one, it just would be pointless after LiveDebugValues has propagated DBG_VALUEs across basic blocks and reached a fixed point.

All true; this is where a question of "what is the design" de-jure and de-facto comes in. Because DbgEntityHistoryCalculator currently relies on LiveDebugValues having run its analysis, does that not _make_ it a semantic change? At the very least, because that's what I saw when trying to document these things, that's what I wrote.

Alternately, we could document the change in interpretation as being an optimisation that DbgEntityHistoryCalculator performs/relies on, rather than being a change in semantics. (I think these are both sides of the same coin when it comes to explaining internal state).

Fri, Sep 20, 1:08 PM · Restricted Project
vsk added a comment to D66526: [utils] Add the llvm-locstats tool.

Yes, please go for it :)

Fri, Sep 20, 12:14 PM · Restricted Project, debug-info
vsk accepted D67717: [DebugInfo] Exclude memory location values as parameter entry values.

lgtm

Fri, Sep 20, 12:14 PM · Restricted Project, debug-info
vsk updated the diff for D67774: [Mangle] Add flag to asm labels to disable global prefixing.

Thanks for your feedback. I've added a flag to asm labels to disable global prefixing. I've tried to minimize the behavior change in this patch -- it seems to me that additional cleanups can happen in follow-ups.

Fri, Sep 20, 11:00 AM

Thu, Sep 19

vsk created D67774: [Mangle] Add flag to asm labels to disable global prefixing.
Thu, Sep 19, 1:02 PM
vsk added a comment to D67717: [DebugInfo] Exclude memory location values as parameter entry values.

Could you update 'lldb/packages/Python/lldbsuite/test/functionalities/param_entry_vals/basic_entry_values_x86_64/main.cpp' in this patch? This should do it:

Thu, Sep 19, 11:39 AM · Restricted Project, debug-info
vsk accepted D67699: [llvm-dwarfdump] Adjust Windows path to be acceptable by JSON.

lgtm, I believe the original llvm-locstats can now land unmodified, which should provide test coverage.

Thu, Sep 19, 11:33 AM · Restricted Project, debug-info
vsk added inline comments to D67770: [Debuginfo] dbg.value points to undef value after Induction Variable Simplification..
Thu, Sep 19, 11:07 AM · debug-info, Restricted Project

Wed, Sep 18

vsk updated subscribers of D67717: [DebugInfo] Exclude memory location values as parameter entry values.
Wed, Sep 18, 1:57 PM · Restricted Project, debug-info
vsk updated subscribers of D67717: [DebugInfo] Exclude memory location values as parameter entry values.
Wed, Sep 18, 11:16 AM · Restricted Project, debug-info
vsk added inline comments to D67699: [llvm-dwarfdump] Adjust Windows path to be acceptable by JSON.
Wed, Sep 18, 10:29 AM · Restricted Project, debug-info
vsk accepted D67563: Debug Info: Add support for named constants.

FTR, I like this approach (introducing DW_AT_LLVM_constant). A side-benefit of not attaching AT_location to TAG_constant is that there's no need to update the dwarf location statistics code. So, +1/lgtm on my end -- would be great to have another +1 on this change though.

Wed, Sep 18, 10:16 AM · Restricted Project, debug-info

Tue, Sep 17

vsk added a comment to D67556: [ARM][AArch64][DebugInfo] Improve call site instruction interpretation.

@dstenb you are right! I've tried this example and it is as you expected. We should avoid dereferencing memory locations. I will address those issues in separate patch.

Tue, Sep 17, 2:21 PM · debug-info
vsk added a comment to D66955: [DebugInfo][If-Converter] Update call site info during the optimization.

Splitting up the updateCallSiteInfo API is a great idea. Some minor comments inline.

Tue, Sep 17, 11:27 AM · debug-info
vsk added inline comments to D66526: [utils] Add the llvm-locstats tool.
Tue, Sep 17, 11:27 AM · Restricted Project, debug-info
vsk added a comment to D67492: [DebugInfo] Add a DW_OP_LLVM_entry_value operation.

Looks reasonable to me, I just suggest clarifying one comment. @aprantl any other outstanding concerns?

Tue, Sep 17, 11:11 AM · Restricted Project, debug-info
vsk accepted D66832: [docs][Bugpoint]Add notes about multiple crashes.

Thanks, this is quite helpful.

Tue, Sep 17, 11:02 AM · Restricted Project

Mon, Sep 16

vsk committed rGc693aa3def01: [test] Clean up previous raw profile before merging into it (authored by vsk).
[test] Clean up previous raw profile before merging into it
Mon, Sep 16, 3:35 PM
vsk committed rG413647d73097: [Coverage] Speed up file-based queries for coverage info, NFC (authored by vsk).
[Coverage] Speed up file-based queries for coverage info, NFC
Mon, Sep 16, 12:10 PM
vsk committed rG95de24978e80: [Coverage] Assert that filenames in a TU are unique, NFC (authored by vsk).
[Coverage] Assert that filenames in a TU are unique, NFC
Mon, Sep 16, 12:09 PM

Fri, Sep 13

vsk created D67575: [Coverage] Speed up file-based queries for coverage info, NFC.
Fri, Sep 13, 3:10 PM · Restricted Project
vsk updated subscribers of D67563: Debug Info: Add support for named constants.

IIUC @jingham makes the argument here that DW_TAG_constant isn't appropriate for swift's 'let', and instead suggests attaching DW_AT_const_value to the DW_TAG_variable (which lldb seems to already understand). Wdyt of that as an alternative?

Fri, Sep 13, 11:24 AM · Restricted Project, debug-info

Thu, Sep 12

vsk accepted D67530: Fix bug in `darwin_test_archs()` when the cache variable is set but empty..

Wow, thanks for chasing this down.

Thu, Sep 12, 6:00 PM · Restricted Project, Restricted Project
vsk accepted D59150: [CMake] Separate the detection Darwin platforms architectures for the built-ins from the rest of compiler-rt..

Lgtm, thanks!

Thu, Sep 12, 5:42 PM · Restricted Project, Restricted Project
vsk added inline comments to D66526: [utils] Add the llvm-locstats tool.
Thu, Sep 12, 5:42 PM · Restricted Project, debug-info

Wed, Sep 11

vsk committed rG21d417dc18ab: [DWARF] Evaluate DW_OP_entry_value (authored by vsk).
[DWARF] Evaluate DW_OP_entry_value
Wed, Sep 11, 2:23 PM
vsk committed rGbb5811852576: [Status] Add a LLDB_ERRORF macro for error reporting (similar to LLDB_LOGF) (authored by vsk).
[Status] Add a LLDB_ERRORF macro for error reporting (similar to LLDB_LOGF)
Wed, Sep 11, 2:23 PM
vsk committed rG0b91333d59f8: [DWARF] Emit call site parameter info when tuning for lldb (authored by vsk).
[DWARF] Emit call site parameter info when tuning for lldb
Wed, Sep 11, 2:23 PM
vsk added a comment to D67410: [DWARF] Emit call site parameter info when tuning for lldb.
In D67410#1666891, @vsk wrote:

The DWARFv5 forms were already the default choice when the tuning isn't set to GDB, this change is about allowing TAG_call_site_parameter to be emitted at all when the tuning is set to LLDB.

*nod*

The idea is that, in the callee, we may now have variables which use DW_OP_entry_value, whereas before the patch that wouldn't have been the case. So the new entry value locations expand the ranges covered by AT_location.

This change (looking at the test changes, at least) looks to be about call_site tags, rather than entry_value?

Anyway, I haven't been paying close attention to the optimized debug info or these DWARFv5 location attributes - so don't mind me. Didn't mean to derail the review or anything.

Wed, Sep 11, 2:06 PM · debug-info, Restricted Project
vsk added a comment to D67410: [DWARF] Emit call site parameter info when tuning for lldb.
In D67410#1666856, @vsk wrote:

I'm very curious what the effect of this patch on our green dragon statistics bot will be (http://green.lab.llvm.org/green/view/LLDB/job/clang-3.4-debuginfo-statistics/).

What sort of statistics do we track that would be impacted by this change?

None for now, because debug entry values are gated behind a CC1 flag the bot does not use.

This change is enabling them by default when tuning for LLDB, right? Or still only when using the cc1 flag /and/ tuning for LLDB?

Wed, Sep 11, 1:53 PM · debug-info, Restricted Project
vsk added a comment to D67410: [DWARF] Emit call site parameter info when tuning for lldb.

I'm very curious what the effect of this patch on our green dragon statistics bot will be (http://green.lab.llvm.org/green/view/LLDB/job/clang-3.4-debuginfo-statistics/).

What sort of statistics do we track that would be impacted by this change?

Wed, Sep 11, 1:35 PM · debug-info, Restricted Project
vsk accepted D67453: Remove the obsolete BlockByRefStruct flag from LLVM IR.

Makes sense, thanks!

Wed, Sep 11, 1:25 PM · Restricted Project, debug-info
vsk added inline comments to D67376: [DWARF] Evaluate DW_OP_entry_value.
Wed, Sep 11, 1:07 PM · Restricted Project
vsk updated the diff for D67376: [DWARF] Evaluate DW_OP_entry_value.
  • Add a working example and some additional comments in Evaluate_DW_OP_entry_value.
Wed, Sep 11, 1:07 PM · Restricted Project
vsk added a comment to D67453: Remove the obsolete BlockByRefStruct flag from LLVM IR.

Looks nice! One question: if DIFlagReservedBit4 is ever reused, will it break bitcode upgrades? Or is the idea that it won't ever be reused?

Wed, Sep 11, 1:05 PM · Restricted Project, debug-info
vsk accepted D67004: [DebugInfo] Enable call site parameter debug info for ARM and AArch64.

Lgtm, sorry for the delay.

Wed, Sep 11, 10:11 AM · debug-info

Tue, Sep 10

vsk added inline comments to D67376: [DWARF] Evaluate DW_OP_entry_value.
Tue, Sep 10, 6:25 PM · Restricted Project
vsk updated the diff for D67376: [DWARF] Evaluate DW_OP_entry_value.
Tue, Sep 10, 6:25 PM · Restricted Project
vsk added inline comments to D67376: [DWARF] Evaluate DW_OP_entry_value.
Tue, Sep 10, 6:15 PM · Restricted Project
vsk updated the diff for D67376: [DWARF] Evaluate DW_OP_entry_value.
  • Fix return address lookup when the immediate parent frame is inlined.
  • Tighten the test so that it actually verifies that tail calls, inlining, etc. occur, instead of assuming :).
  • Add/move various TODOs as pointed out by reviewers.
Tue, Sep 10, 6:14 PM · Restricted Project
vsk planned changes to D67376: [DWARF] Evaluate DW_OP_entry_value.

While tightening up the test case I think I found an issue with the way inlined frames are handled. I need to take a closer look.

Tue, Sep 10, 3:33 PM · Restricted Project
vsk updated subscribers of D67356: [InstCombine] Simplify @llvm.usub.with.overflow+non-zero check (PR43251).
Tue, Sep 10, 1:42 PM · Restricted Project
vsk updated subscribers of D66832: [docs][Bugpoint]Add notes about multiple crashes.

I think it would be great to make the powerful bugpoint tool easier to use, especially for beginners, for common simple scenarios.

But if we don't think this is really necessary or in the wrong direction of design, I am OK to abandon this patch.

Tue, Sep 10, 12:28 PM · Restricted Project
vsk added inline comments to D67356: [InstCombine] Simplify @llvm.usub.with.overflow+non-zero check (PR43251).
Tue, Sep 10, 12:11 PM · Restricted Project
vsk committed rG3ef7dbd6650d: [lldbtest] Add an "expected_cmd_failure" option to the filecheck helper (authored by vsk).
[lldbtest] Add an "expected_cmd_failure" option to the filecheck helper
Tue, Sep 10, 11:37 AM
vsk committed rGff02109ad47a: [Function] Factor out GetCallEdgeForReturnAddress, NFC (authored by vsk).
[Function] Factor out GetCallEdgeForReturnAddress, NFC
Tue, Sep 10, 11:37 AM
vsk added inline comments to D67376: [DWARF] Evaluate DW_OP_entry_value.
Tue, Sep 10, 11:23 AM · Restricted Project
vsk updated the diff for D67376: [DWARF] Evaluate DW_OP_entry_value.
  • Addressed review feedback, split out unrelated changes, and improved test coverage.
Tue, Sep 10, 11:21 AM · Restricted Project
vsk created D67410: [DWARF] Emit call site parameter info when tuning for lldb.
Tue, Sep 10, 11:13 AM · debug-info, Restricted Project

Mon, Sep 9

vsk planned changes to D67376: [DWARF] Evaluate DW_OP_entry_value.

TODO:

  • Split out llvm change.
  • Add a test to validate that lldb skips inline frames when evaluating DW_OP_entry_value.
Mon, Sep 9, 6:15 PM · Restricted Project
vsk updated the diff for D67376: [DWARF] Evaluate DW_OP_entry_value.
  • Partially address review feedback.
Mon, Sep 9, 6:15 PM · Restricted Project
vsk added a comment to D67369: Implement DW_OP_convert.

Looks good overall, especially the testing.

Mon, Sep 9, 5:45 PM · Restricted Project, Restricted Project
vsk added a comment to D66832: [docs][Bugpoint]Add notes about multiple crashes.

I think you've identified a real, pernicious problem, but am not sure that this is the best approach for addressing it. One potential problem here is that slight variations in the error message can confuse the reducer. If e.g. some location information changes in between messages, the reduction will stop too early.

Mon, Sep 9, 5:24 PM · Restricted Project
vsk accepted D67378: [Utility] Replace Cleanup by llvm::scope_exit.

Thanks a lot!

Mon, Sep 9, 5:13 PM · Restricted Project, Restricted Project
vsk added inline comments to D67356: [InstCombine] Simplify @llvm.usub.with.overflow+non-zero check (PR43251).
Mon, Sep 9, 3:59 PM · Restricted Project
vsk updated the diff for D67376: [DWARF] Evaluate DW_OP_entry_value.
  • Clean up the test.
Mon, Sep 9, 3:36 PM · Restricted Project
vsk created D67376: [DWARF] Evaluate DW_OP_entry_value.
Mon, Sep 9, 3:32 PM · Restricted Project
vsk added inline comments to D67356: [InstCombine] Simplify @llvm.usub.with.overflow+non-zero check (PR43251).
Mon, Sep 9, 1:25 PM · Restricted Project
vsk added inline comments to D67356: [InstCombine] Simplify @llvm.usub.with.overflow+non-zero check (PR43251).
Mon, Sep 9, 1:02 PM · Restricted Project
vsk added a comment to D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour.
In D67122#1659721, @vsk wrote:

Still think this looks good. Have you tried running this on the llvm test suite, or some other interesting corpus? Would be curious to see any pre/post patch numbers.

There were 1.5 occurrences in test-suite.

I'd prefer not to try running it over llvm stage-2/3 (i can, but it's gonna be slow..)

Though i guess you were talking about *performance* numbers?

Mon, Sep 9, 12:52 PM · Restricted Project, Restricted Project, Restricted Project

Fri, Sep 6

vsk accepted D67225: [DebugInfo][X86] Describe call site values for zero-valued imms.

Lgtm.

Fri, Sep 6, 9:15 AM · Restricted Project, debug-info
vsk accepted D67261: [NFC] Make the describeLoadedValue() hook return machine operand objects.

Lgtm as well, thanks.

Fri, Sep 6, 9:03 AM · Restricted Project, debug-info

Thu, Sep 5

vsk added a comment to D67225: [DebugInfo][X86] Describe call site values for zero-valued imms.

Thanks for working on this!

Thu, Sep 5, 3:41 PM · Restricted Project, debug-info
vsk accepted D66525: [llvm-dwarfdump] Add additional stats fields.

Thanks, lgtm!

Thu, Sep 5, 2:34 PM · Restricted Project, debug-info
vsk committed rG1261f1b980d4: [libcxx] Codesign test executables if necessary (authored by vsk).
[libcxx] Codesign test executables if necessary
Thu, Sep 5, 2:26 PM
vsk added a comment to D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour.

Still think this looks good. Have you tried running this on the llvm test suite, or some other interesting corpus? Would be curious to see any pre/post patch numbers.

Thu, Sep 5, 1:03 PM · Restricted Project, Restricted Project, Restricted Project

Wed, Sep 4

vsk updated subscribers of D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour.

Thanks, this is looking pretty good.

Wed, Sep 4, 11:38 AM · Restricted Project, Restricted Project, Restricted Project

Tue, Sep 3

vsk added inline comments to D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour.
Tue, Sep 3, 4:47 PM · Restricted Project, Restricted Project, Restricted Project
vsk committed rG0fcfe8971798: [llvm-profdata] Add mode to recover from profile read failures (authored by vsk).
[llvm-profdata] Add mode to recover from profile read failures
Tue, Sep 3, 3:23 PM
vsk committed rG95fb23ab37e5: [InstrProf] Tighten a check for malformed data records in raw profiles (authored by vsk).
[InstrProf] Tighten a check for malformed data records in raw profiles
Tue, Sep 3, 3:23 PM
vsk added inline comments to D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour.
Tue, Sep 3, 3:20 PM · Restricted Project, Restricted Project, Restricted Project
vsk updated the diff for D66985: [llvm-profdata] Add mode to recover from profile read failures.
  • Shorten failure mode names.
Tue, Sep 3, 1:26 PM · Restricted Project
vsk added a comment to D66525: [llvm-dwarfdump] Add additional stats fields.

Looks mostly good to me!

Tue, Sep 3, 1:19 PM · Restricted Project, debug-info
vsk accepted D66526: [utils] Add the llvm-locstats tool.

Thanks, this looks good to me. Perhaps you can get more feedback by dropping the WIP label. @aprantl, any thoughts?

Tue, Sep 3, 1:13 PM · Restricted Project, debug-info
vsk updated the diff for D66985: [llvm-profdata] Add mode to recover from profile read failures.
  • Allow users to pick the failure mode for profile merging.
Tue, Sep 3, 12:13 PM · Restricted Project
vsk added a comment to D66985: [llvm-profdata] Add mode to recover from profile read failures.

yes, the old behavior should be the default. It raises the awareness of the problem (and merge errors are usually rare in reality).

Tue, Sep 3, 10:00 AM · Restricted Project
vsk added inline comments to D66979: [InstrProf] Tighten a check for malformed data records in raw profiles.
Tue, Sep 3, 10:00 AM · Restricted Project
vsk added a comment to D66985: [llvm-profdata] Add mode to recover from profile read failures.

@davidxl Are you recommending that the old behavior (fail the merge if any profiles are malformed) should remain the default? I think that's reasonable (it raises the alarm about toolchain bugs), just wanted to double-check.

Tue, Sep 3, 9:18 AM · Restricted Project
vsk updated subscribers of D66979: [InstrProf] Tighten a check for malformed data records in raw profiles.
Tue, Sep 3, 9:12 AM · Restricted Project

Fri, Aug 30

vsk added inline comments to D66526: [utils] Add the llvm-locstats tool.
Fri, Aug 30, 9:31 AM · Restricted Project, debug-info
vsk accepted D66955: [DebugInfo][If-Converter] Update call site info during the optimization.

FWIW, this looks good to me, but it would be great to get another +1 as I'm not familiar with this pass.

Fri, Aug 30, 9:23 AM · debug-info
vsk added a comment to D67004: [DebugInfo] Enable call site parameter debug info for ARM and AArch64.

Update test/CodeGen/debug-info-param-modification.c?

Fri, Aug 30, 9:19 AM · debug-info
vsk added a comment to D66979: [InstrProf] Tighten a check for malformed data records in raw profiles.

Pardon my ignorance, but what does rdar://54843625 mean? I guess it's not http://openradar.appspot.com/54843625 ? Is it something I can access? :)

Fri, Aug 30, 9:07 AM · Restricted Project
vsk planned changes to D66985: [llvm-profdata] Add mode to recover from profile read failures.

Need to add an option to preserve the old error reporting behavior. I'll get back to this on Tuesday.

Fri, Aug 30, 9:07 AM · Restricted Project
vsk planned changes to D66979: [InstrProf] Tighten a check for malformed data records in raw profiles.

Thanks for your feedback. I will address review comments and continue investigating on Tuesday.

Fri, Aug 30, 8:55 AM · Restricted Project

Thu, Aug 29

vsk updated the diff for D66985: [llvm-profdata] Add mode to recover from profile read failures.
  • Don't record a read failure when a soft error is encountered within mergeRecordsFromWriter.
Thu, Aug 29, 5:49 PM · Restricted Project
vsk updated the diff for D66985: [llvm-profdata] Add mode to recover from profile read failures.
  • Fix a comment.
Thu, Aug 29, 5:35 PM · Restricted Project
vsk created D66985: [llvm-profdata] Add mode to recover from profile read failures.
Thu, Aug 29, 5:27 PM · Restricted Project
vsk updated the diff for D66979: [InstrProf] Tighten a check for malformed data records in raw profiles.
  • Bring back validation of NumCounters.
Thu, Aug 29, 4:09 PM · Restricted Project
vsk planned changes to D66979: [InstrProf] Tighten a check for malformed data records in raw profiles.

This mistakenly drops validation for NumCounters...

Thu, Aug 29, 3:45 PM · Restricted Project
vsk created D66979: [InstrProf] Tighten a check for malformed data records in raw profiles.
Thu, Aug 29, 3:42 PM · Restricted Project
vsk added inline comments to D66955: [DebugInfo][If-Converter] Update call site info during the optimization.
Thu, Aug 29, 2:51 PM · debug-info
vsk added inline comments to D66955: [DebugInfo][If-Converter] Update call site info during the optimization.
Thu, Aug 29, 11:23 AM · debug-info
vsk accepted D66953: [ISEL][ARM][AARCH64] Tracking simple parameter forwarding registers.

Thanks, I think this is a great starting point. LGTM.

Thu, Aug 29, 11:03 AM · debug-info
vsk accepted D66888: [LiveDebugValues] Insert entry values after bundles.

LGTM

Thu, Aug 29, 11:00 AM · Restricted Project, debug-info
vsk updated the diff for D66496: [libcxx] Codesign test executables if necessary.
  • Address coding style issue in run.py.
Thu, Aug 29, 10:32 AM · Restricted Project
vsk added inline comments to D66496: [libcxx] Codesign test executables if necessary.
Thu, Aug 29, 10:22 AM · Restricted Project

Wed, Aug 28

vsk added inline comments to D66526: [utils] Add the llvm-locstats tool.
Wed, Aug 28, 1:46 PM · Restricted Project, debug-info
Herald added a project to D42712: [utils] Add utils/update_cc_test_checks.py: Restricted Project.
Wed, Aug 28, 10:46 AM · Restricted Project