vsk (Vedant Kumar)
User

Projects

User Details

User Since
Jul 8 2015, 10:26 AM (132 w, 18 h)

Recent Activity

Yesterday

vsk updated the diff for D42215: [CMake] Make check-lldb work with LLDB_CODESIGN_IDENTITY=''.

Thanks for the feedback!

Wed, Jan 17, 5:04 PM
vsk created D42215: [CMake] Make check-lldb work with LLDB_CODESIGN_IDENTITY=''.
Wed, Jan 17, 4:46 PM
vsk added a comment to D42206: If kevent() is interrupted by signal (or is being debugged) and we get EINTR, retry.

Are we going to test each unix call that can fail with EINTR? Seems a bit overkill.

Wed, Jan 17, 4:28 PM · Restricted Project
vsk added a comment to D42206: If kevent() is interrupted by signal (or is being debugged) and we get EINTR, retry.

I tried sending signals to lldb-server via kill() and the signal handler caught them, the bit of code I had printing out the return value & errno value never got executed. The only way I was able to repo this was by debugging lldb-server.

Wed, Jan 17, 4:07 PM · Restricted Project
vsk requested changes to D42206: If kevent() is interrupted by signal (or is being debugged) and we get EINTR, retry.

Why not take an approach similar to the one in https://reviews.llvm.org/D41008? It looks like it's possible to set up a poll loop, call signal(), and verify that the loop is still running.

Wed, Jan 17, 3:57 PM · Restricted Project
vsk added reviewers for D42043: c-index: CXString: fix MSAN read-past-end bug: akyrtzi, benlangmuir.
Wed, Jan 17, 3:32 PM

Tue, Jan 16

vsk updated the diff for D41921: [Parse] Forward brace locations to TypeConstructExpr.
Tue, Jan 16, 1:34 PM

Fri, Jan 12

vsk added reviewers for D41976: Low-hanging fruit optimization in string::__move_assign().: EricWF, mclow.lists, hiraditya.

Adding some folks who may be interested.

Fri, Jan 12, 1:09 PM
vsk accepted D42000: [llvm-cov] Skip unnecessary coverage computations for "export -summary-only"..

Looks good, thanks!

Fri, Jan 12, 12:21 PM

Thu, Jan 11

vsk added a comment to D41971: MC: Remove redundant `SetUsed` arguments in MCSymbol methods. NFC.
In D41971#974111, @vsk wrote:

Your changes to lib/ lgtm.

IMO getting rid of the SetUsed parameters entirely would be preferable to having only some predicates accept SetUsed. Have you considered removing the update to IsUsed in getVariableValue(), and inserting calls to setUsed() as needed to fix any regressions?

I think you are probably right, but I didn't want to change AsmParser.cpp too much because I don't know that part of the codebase at all. This change made sense to me because I can easily see that it is a NFC. Are you suggesting that AsmParser.cpp rely on calling setUsed explicitly rather than implicitly? I think that would probably be more clear.

Thu, Jan 11, 5:02 PM
vsk added a reviewer for D41971: MC: Remove redundant `SetUsed` arguments in MCSymbol methods. NFC: rafael.
Thu, Jan 11, 4:52 PM
vsk added a comment to D41971: MC: Remove redundant `SetUsed` arguments in MCSymbol methods. NFC.

Your changes to lib/ lgtm.

Thu, Jan 11, 4:51 PM

Wed, Jan 10

vsk accepted D41924: dagcombine: Transfer debug information when folding (zext (truncate x)) -> (zext (truncate x)).

Looks good.

Wed, Jan 10, 5:22 PM
vsk created D41921: [Parse] Forward brace locations to TypeConstructExpr.
Wed, Jan 10, 3:22 PM

Mon, Jan 8

vsk updated the diff for D41793: [Debugify] Add a mode to opt to enable faster testing.
Mon, Jan 8, 6:56 PM · debug-info
vsk added a comment to D41793: [Debugify] Add a mode to opt to enable faster testing.

I think @MatzeB's suggestion works well without requiring any test changes. I'll update this patch soon.

Mon, Jan 8, 6:45 PM · debug-info

Fri, Jan 5

vsk created D41793: [Debugify] Add a mode to opt to enable faster testing.
Fri, Jan 5, 4:48 PM · debug-info
vsk added a comment to D41496: [EarlyCSE] Salvage debug info during DCE.

I think the test case could probably be reduced a bit (is 'ctpop' necessary?), but apart from that, this looks good. @aprantl wdyt?

Fri, Jan 5, 3:20 PM
vsk added a comment to D41787: [Utils] Simplify salvageDebugInfo, NFCI.

Are there any remaining users of findDebugValues that don't actually want to use find DebugUsers?

Fri, Jan 5, 3:16 PM · debug-info
vsk created D41787: [Utils] Simplify salvageDebugInfo, NFCI.
Fri, Jan 5, 2:51 PM · debug-info
vsk added a comment to D41776: [lit] Implement "-r" option for builtin "diff" command + a test using that..

I haven't looked too closely, but I think this test might fail:

Fri, Jan 5, 10:44 AM

Wed, Jan 3

vsk accepted D41600: [llvm-cov] Refactor "export" command implementation and add support for SOURCES..

LGTM.

Wed, Jan 3, 4:32 PM
vsk accepted D41206: [llvm-cov] Multi-threaded implementation of prepareFileReports method..

Thanks, LGTM.

Wed, Jan 3, 4:08 PM
vsk added a comment to D40727: Syndicate duplicate code between CallInst and InvokeInst.

This looks like a nice cleanup.

Wed, Jan 3, 3:07 PM
vsk created D41717: [CGBuiltin] Handle unsigned mul overflow properly (PR35750).
Wed, Jan 3, 2:33 PM

Thu, Dec 21

vsk added inline comments to D41206: [llvm-cov] Multi-threaded implementation of prepareFileReports method..
Thu, Dec 21, 2:50 PM

Wed, Dec 20

vsk added a comment to D41044: Implementation of -fextend-lifetimes and -fextend-this-ptr to aid with debugging of optimized code.

I have some results from the development build of our kernel ('-O2 -g -flto'). According to dwarfdump -statistics, when compiled with -fextend-lifetimes, the percentage of covered scope bytes increases from 62% to 69%. The number of inlined scopes decreases by 4%, and (I think relatedly) the size of the binary increases by 14%. There is a small increase in the number of unique source variables (under 1%). I'd be happy to report back on any other suggested quantitative measures.

Wed, Dec 20, 3:08 PM · debug-info
vsk accepted D41461: Add hasProfileData() to check if a function has profile data. NFC..

Looks like a nice cleanup to me.

Wed, Dec 20, 1:05 PM
vsk accepted D40295: -fsanitize=vptr warnings on bad static types in dynamic_cast and typeid.

Thanks, lgtm. Could you wait a day or two before committing? IIRC Richard or Nico have a -fsanitize=vptr Chromium bot, and they may have further comments.

Wed, Dec 20, 11:39 AM
vsk added a comment to D41206: [llvm-cov] Multi-threaded implementation of prepareFileReports method..

For a test, I suggest creating a report for >1 files with/without threading, and asserting that the reports are identical. It would be good to exercise the new summary aggregation logic by including different instantiations of the same template function in >1 files.

Wed, Dec 20, 11:32 AM

Tue, Dec 19

vsk added a comment to D40295: -fsanitize=vptr warnings on bad static types in dynamic_cast and typeid.

I don't think any checks can be skipped in the newly-introduced calls to EmitTypeCheck. Clang uses EmitDynamicCast on arbitrary addresses, not just addresses which are known to be checked for alignment/etc. Regarding the test update, I think it makes sense to extend the runtime test in vptr.cpp, but that we'd also benefit from a small/narrow IR test (e.g in test/CodeGenCXX/ubsan-vtable-checks.cpp). With the added test I think this patch would be in great shape.

Tue, Dec 19, 12:03 PM

Dec 18 2017

vsk accepted D41374: [Coverage] Fix use-after free in coverage emission.

Thanks, this lgtm as a stopgap.

Dec 18 2017, 5:43 PM · Restricted Project
vsk updated the diff for D40698: [ubsan] Diagnose noreturn functions which return.
  • Tighten the IR test case.
Dec 18 2017, 3:44 PM
vsk updated the diff for D40698: [ubsan] Diagnose noreturn functions which return.
  • Make the patch cleaner by introducing an overload of EmitCall() which doesn't require a SourceLocation argument.
Dec 18 2017, 3:27 PM
vsk added a comment to D40720: No -fsanitize=function warning when calling noexcept function through non-noexcept pointer in C++17.
In D40720#958677, @vsk wrote:

Please add a test.

Note that the bot upon the first closing of this review changed the shown diff from the combined cfe+compiler-rt diff to just the cfe part. See https://reviews.llvm.org/rL320977 for the compiler-rt part, including tests in compiler-rt/trunk/test/ubsan/TestCases/TypeCheck/Function/function.cpp.

Dec 18 2017, 1:37 PM
vsk added a comment to D40720: No -fsanitize=function warning when calling noexcept function through non-noexcept pointer in C++17.

Copying my comment from the diffusion thread to keep things in one place:

You could make FunctionTypeMismatch an 'AlwaysRecoverable' check, just like the Vptr check, and remove the _abort handlers from the ubsan runtimes.

Dec 18 2017, 11:50 AM
vsk added a comment to D40720: No -fsanitize=function warning when calling noexcept function through non-noexcept pointer in C++17.

Please add a test.

Dec 18 2017, 11:49 AM
vsk added a comment to rCRT320977: No -fsanitize=function warning when calling noexcept function through non….

You could make FunctionTypeMismatch an 'AlwaysRecoverable' check, just like the Vptr check, and remove the _abort handlers from the ubsan runtimes.

Dec 18 2017, 11:47 AM
vsk added a comment to D34499: Expose __gcov_flush for parity with libgcov in the gcc project.

Here's a good example of how to write a test for this sort of thing: test/profile/instrprof-reset-counters.c

Dec 18 2017, 11:07 AM
vsk added a comment to D34499: Expose __gcov_flush for parity with libgcov in the gcc project.

Sorry this slipped through the cracks. It's OK to ping patches weekly to make sure they get attention.

Dec 18 2017, 10:58 AM

Dec 15 2017

vsk added a comment to D41149: [CodeGen] Specialize mixed-sign mul-with-overflow (fix PR34920).

Thanks for the review!

Dec 15 2017, 5:21 PM

Dec 14 2017

vsk accepted D41222: Handle previously ASAN-instrumented IR gracefully when ASAN re-invoked.

Really nice, lgtm!

Dec 14 2017, 10:39 AM

Dec 13 2017

vsk added a comment to D41222: Handle previously ASAN-instrumented IR gracefully when ASAN re-invoked.

Is it possible to reduce the test case s.t the test file doesn't contain instrumented IR? ASan instrumentation is prone to change, so I worry that the test could start passing for invalid reasons. This might happen if 1) __asan_before_dynamic_init is renamed, 2) GlobalsMetadata::doInit is written more defensively, and 3) a non-idempotent ASan transform is introduced later.

Dec 13 2017, 8:17 PM
vsk updated the summary of D41149: [CodeGen] Specialize mixed-sign mul-with-overflow (fix PR34920).
Dec 13 2017, 3:37 PM
vsk added inline comments to D41149: [CodeGen] Specialize mixed-sign mul-with-overflow (fix PR34920).
Dec 13 2017, 3:33 PM
vsk updated the diff for D41149: [CodeGen] Specialize mixed-sign mul-with-overflow (fix PR34920).
Dec 13 2017, 3:32 PM

Dec 12 2017

vsk updated the diff for D41149: [CodeGen] Specialize mixed-sign mul-with-overflow (fix PR34920).
  • Make sure the result can be stored into the result ptr.
Dec 12 2017, 6:08 PM
vsk abandoned D38861: [CodeGen] Error on unsupported checked multiplies early.

Abandoned in favor of a proper fix: https://reviews.llvm.org/D41149

Dec 12 2017, 5:57 PM
vsk created D41149: [CodeGen] Specialize mixed-sign mul-with-overflow (fix PR34920).
Dec 12 2017, 5:55 PM
vsk accepted D41140: [Coverage] Always emit unused coverage mappings in the same order..

Thanks, lgtm!

Dec 12 2017, 4:08 PM · Restricted Project
vsk added a comment to D41111: [profile] Solaris ld supports __start___llvm_prof_data etc. labels.

Is there a way to gate the change on the Solaris OS version (maybe it's in the triple somewhere)? Apart from that, this patch lgtm.

Dec 12 2017, 10:39 AM
vsk accepted D40944: [profile] Enable on Solaris.

Thanks! LGTM with a nit -- please add the comment we discussed in InstrProfilingPort.h.

Dec 12 2017, 10:37 AM

Dec 11 2017

vsk accepted D41101: [testsuite] Remove testing failures vestiges.

Thanks! The 6.0 compiler is ancient at this point. This lgtm.

Dec 11 2017, 5:10 PM
vsk added inline comments to D41085: [llvm-cov] Add an option for "export" command to emit only file summary data..
Dec 11 2017, 3:36 PM
vsk accepted D41085: [llvm-cov] Add an option for "export" command to emit only file summary data..

Thanks Max! Lgtm with a basic test, e.g;

Dec 11 2017, 12:51 PM
vsk added a project to D41044: Implementation of -fextend-lifetimes and -fextend-this-ptr to aid with debugging of optimized code: debug-info.

Thanks for your patches @wolfgangp! I agree with Eli that we should evaluate enabling this automatically with -Og. I'll test this out on a few internal projects and report back.

Dec 11 2017, 11:51 AM · debug-info

Dec 8 2017

vsk updated the diff for D39982: [IRBuilder] Set the insert point and debug location together.
  • Do not mark any C APIs as deprecated.
Dec 8 2017, 2:30 PM · debug-info
vsk added inline comments to D39982: [IRBuilder] Set the insert point and debug location together.
Dec 8 2017, 2:26 PM · debug-info
vsk added inline comments to D40944: [profile] Enable on Solaris.
Dec 8 2017, 1:27 PM
vsk added a comment to D40940: [ubsan] Use pass_object_size info in bounds checks.

I backed out the part of this patch which deals with array parameters declared like p[10] or p[static 10]: r320185.

Dec 8 2017, 11:52 AM
vsk closed D40940: [ubsan] Use pass_object_size info in bounds checks.

Landed in r320128.

Dec 8 2017, 11:26 AM
vsk closed D40941: [ubsan] Use pass_object_size info in bounds checks (compiler-rt).

Landed in r320129.

Dec 8 2017, 11:26 AM

Dec 7 2017

vsk added a comment to D40668: [Blocks] Inherit sanitizer options from parent decl.

LGTM.

Thanks!

I was wondering if there are other places where this propagation needs to be added, for example, other calls to GenerateBlockFunction.

Dec 7 2017, 6:43 PM
vsk added a comment to D40512: [Debugify] Add a pass to test debug info preservation.

Thanks everybody for your feedback. I think I've addressed all of it now. I'll commit this tomorrow barring any further comments.

Dec 7 2017, 4:45 PM · debug-info
vsk added inline comments to D40941: [ubsan] Use pass_object_size info in bounds checks (compiler-rt).
Dec 7 2017, 4:27 PM
vsk added a comment to D40668: [Blocks] Inherit sanitizer options from parent decl.

Friendly ping.

Dec 7 2017, 3:51 PM
vsk updated the diff for D40940: [ubsan] Use pass_object_size info in bounds checks.
  • Handle constant size modifiers while we're at it (e.g "int foo(int p[static 10])").
Dec 7 2017, 3:45 PM
vsk updated the diff for D40940: [ubsan] Use pass_object_size info in bounds checks.

Thanks for your feedback.

Dec 7 2017, 1:55 PM
vsk added a reviewer for D40944: [profile] Enable on Solaris: vsk.
Dec 7 2017, 1:17 PM

Dec 6 2017

vsk created D40941: [ubsan] Use pass_object_size info in bounds checks (compiler-rt).
Dec 6 2017, 7:46 PM
vsk created D40940: [ubsan] Use pass_object_size info in bounds checks.
Dec 6 2017, 7:45 PM
vsk added inline comments to D39982: [IRBuilder] Set the insert point and debug location together.
Dec 6 2017, 2:49 PM · debug-info
vsk updated the diff for D40757: Disable warnings related to anonymous types in the ObjC plugin.
  • Thanks @labath! I've disabled the GNU warnings in a narrower way. PTAL.
Dec 6 2017, 2:23 PM
vsk accepted D40873: [PGO] handle infinite loop properly in pgo instrumention.

What happens is that the call of getAnalysis<LoopInfoWrapperPass>() is invoked after getAnalysis<BlockFrequencyInfoWrapperPass>(). The legacy pass manager in this case will do the analysis run to compute LoopInfo on the fly. Before running the analysis, it will also invoke the 'release' memory method of the analysis passes contained in, in this case is BPI and BFI. After that we basically have use-after free problem. The leaking is probably just a side effect.

Dec 6 2017, 11:32 AM
vsk added a comment to D40873: [PGO] handle infinite loop properly in pgo instrumention.

I don't have my head wrapped around the cause of the leak yet. Was the issue that getAnalysis<LoopInfo...>() recomputed BFI, thereby leaking the existing analysis? If so, would it be sensible for the pass manager to provide a cached BFI instead of allowing the leak? (I'm not suggesting changing the PM, just curious about how these things fit together.)

Dec 6 2017, 11:19 AM

Dec 5 2017

vsk added a comment to D40778: [DebugIR] Revive the Debug IR pass. [Added llvm-commits].

Regarding StripDebugInfo, I did not know it existed. I should refactor the code to use it, I assume? I'll check this out.

Dec 5 2017, 5:34 PM · debug-info
vsk added a reviewer for D40821: Fix const-correctness in RegisterContext methods, NFC: clayborg.
Dec 5 2017, 3:36 PM
vsk updated the diff for D40821: Fix const-correctness in RegisterContext methods, NFC.
  • Address Greg's comments.
Dec 5 2017, 3:36 PM
vsk added inline comments to D40512: [Debugify] Add a pass to test debug info preservation.
Dec 5 2017, 1:57 PM · debug-info
vsk updated the diff for D40512: [Debugify] Add a pass to test debug info preservation.
  • Address comments by Adrian and Matthias.
Dec 5 2017, 1:57 PM · debug-info
vsk added inline comments to D40512: [Debugify] Add a pass to test debug info preservation.
Dec 5 2017, 1:25 PM · debug-info
vsk updated the diff for D40512: [Debugify] Add a pass to test debug info preservation.
  • Make the return type in a lambda explicit, & a minor tweak to an assert.
Dec 5 2017, 1:25 PM · debug-info
vsk updated the diff for D40512: [Debugify] Add a pass to test debug info preservation.
  • Move Debugify* to tools/opt, which simplifies the patch and avoids bloating llvm. Thanks @MatzeB for the suggestion!
Dec 5 2017, 1:09 PM · debug-info
vsk added a comment to D40821: Fix const-correctness in RegisterContext methods, NFC.

Seems wrong to remove the const on structs that don't need to change in order to make the write happen. Can't we just quiet the warnings with a const_cast inside the function call?

Dec 5 2017, 11:00 AM

Dec 4 2017

vsk created D40821: Fix const-correctness in RegisterContext methods, NFC.
Dec 4 2017, 5:43 PM
vsk added a comment to D40812: Remove no-op null checks, NFC.

Ah, sorry, I should have mentioned that the header vending compression_decode_buffer() does not provide the 'weak_import' attribute (as far as I can see!). If you change your example to drop 'weak_import' from foo, I think it will be closer to the current situation.

Dec 4 2017, 4:27 PM
vsk created D40812: Remove no-op null checks, NFC.
Dec 4 2017, 4:08 PM
vsk added a comment to D40757: Disable warnings related to anonymous types in the ObjC plugin.

If the "excuse" for not following llvm here is that these structs mirror apple headers, should we restrict these warnings only to the relevant files (e.g. everything in source/Plugins/Language/ObjC)?

I don't particularly care about whether we do, but I wanted to throw the idea out there, as this is the reason why I haven't done something similar yet (although the warnings are quite annoying).

Dec 4 2017, 3:01 PM
vsk added a comment to D40778: [DebugIR] Revive the Debug IR pass. [Added llvm-commits].
In D40778#944145, @vsk wrote:

Personally, I'm usually debugging JITd code, so there is no natural *.ll file on disk, and at no point is IR parsed from text. It's my understanding that debuggers do generally expect a source file to exist, but it would still be more natural for me to generate the source file on the side, rather than writing it out and then immediately using it to reconstruct the module.

I don't think this use case would be supported unless -debugir would always be a part of the JIT pipeline. If debugging needs to be enabled by a special flag, it would be simple enough to implement it by invoking llvm-as on M.dump(). Implementing -debugir in the LL reader has other advantages, e.g it allows you to mark up the LL file with comments or edit it, then re-compile & debug again.

To be clear, I'm not using the off-the-shelf JIT pipeline, but rather generating code and wrangling RuntimeDyld and friends myself. Hence, a pass would work well for me. While your proposed approach could be made to work for my case, it would be significantly less convenient, and would not let me take advantage of any opportunity to add comments to IR for debugging convenience (although I have never felt any need to do so).

Dec 4 2017, 2:01 PM · debug-info
vsk added a comment to D40778: [DebugIR] Revive the Debug IR pass. [Added llvm-commits].
In D40778#944100, @vsk wrote:
  • Should debugIR be a pass? The main use seems to be: there is an existing *.ll file on disk, and it needs to be debugged. Doesn't it make more sense to add a cl::opt to the LL reader to enable debugIR, so that the existing file can be debugged in-place?

Personally, I'm usually debugging JITd code, so there is no natural *.ll file on disk, and at no point is IR parsed from text. It's my understanding that debuggers do generally expect a source file to exist, but it would still be more natural for me to generate the source file on the side, rather than writing it out and then immediately using it to reconstruct the module.

Dec 4 2017, 1:25 PM · debug-info
vsk added a comment to D39982: [IRBuilder] Set the insert point and debug location together.

Friendly ping. I've outlined my thoughts on deprecating the old API versus updating the existing SetInsertPoint users above. Does anyone have feedback about that?

Dec 4 2017, 1:09 PM · debug-info
vsk requested changes to D40778: [DebugIR] Revive the Debug IR pass. [Added llvm-commits].

Thanks for resubmitting this @bollu. I discussed this idea with @Ralith, @rkruppe, @aprantl and others. I have a better notion now of how debugIR could be helpful for optimization and frontend authors.

Dec 4 2017, 12:54 PM · debug-info
vsk accepted D40777: [dsymutil] Add -verify option to run DWARF verifier after linking..

Looks good to me.

Dec 4 2017, 12:03 PM
vsk added a comment to D40702: [PGO] More fix to infinite loop profiling.

This fix lgtm, but I've never touched this code, so it'd be worth getting another +1.

Dec 4 2017, 11:58 AM

Dec 1 2017

vsk created D40757: Disable warnings related to anonymous types in the ObjC plugin.
Dec 1 2017, 4:35 PM
vsk added a comment to D40745: Add a clang-ast subcommand to lldb-test.

The general approach sgtm, and the patch itself looks reasonable.

Dec 1 2017, 3:19 PM
vsk added a reviewer for D40698: [ubsan] Diagnose noreturn functions which return: efriedma.
Dec 1 2017, 1:53 PM
vsk updated the diff for D40698: [ubsan] Diagnose noreturn functions which return.
  • Diagnose in the scenario Eli pointed out, by stripping the 'noreturn' attribute and emitting a check after the call.
  • Test updates.
Dec 1 2017, 1:52 PM
vsk updated the diff for D40700: [ubsan] Diagnose noreturn functions which return (compiler-rt).
  • Update to test diagnostics seen after a call to the noreturn function.
Dec 1 2017, 1:50 PM

Nov 30 2017

vsk planned changes to D40698: [ubsan] Diagnose noreturn functions which return.
Nov 30 2017, 7:19 PM