- User Since
- Mar 28 2019, 8:58 AM (115 w, 2 d)
Tue, May 25
Mon, May 24
Fri, May 21
It seems odd that not trimming the locations would have a positive impact on coverage since, IIUC, the only difference should be an increase in bytes covered outside the lexical scope (which is not counted towards "PC ranges covered" as of D85636). I can see that in the tests you've updated in this patch some DBG_VALUEs are moved from just outside to inside the prologue. I have vague memories from my time working on D82129 that LexicalScope::getRanges doesn't include the prologue in any range or something like that. I'm unsure whether that is intended behaviour or not - I would be interested if anyone knows? I think my solution in D82129 was similarly to avoid trimming the location ranges for function-scoped variables.
+ Rebased on the latest version of D99651 (which has been approved now, so should be its final form)
Hi, before this lands you will need to update your dexter tests to use the new label reference syntax added in D101147. You can do so manually, or alternatively use this python script by running python make_label_refs.py debuginfo-tests/feature_tests (please note it will modify files in place). I'm happy for you to amend your commit with that update without bringing it back up here for review.
+ Changed is_one_shot to hit_count. Updated patch title and summary.
Wed, May 19
It looks like you've addressed both of our comments (except my suggestion for a non-canonical path spelling test for windows exclusively, which would still be welcomed but isn't critical) so LGTM. I'll leave it to @jmorse to Accept this if he's happy.
Mon, May 17
May 6 2021
+ Replace two GetID() calls with id in delete_breakpoint.
May 5 2021
Thanks for taking a look.
Alternatively we could a slightly more general keyword arg hit_count=<int> that limits the times the breakpoints can be triggered to any number, rather than just once. I'm not sure we need it right now, but it's more flexible. Does anyone have any opinion on this?
Apr 30 2021
LGTM but please wait a little to see if anyone else has comments on the test, cc @jmorse.
Apr 29 2021
The change makes sense and LGTM.
+ Update Commands.md
+ Add REQUIRES: system-linux in test (DexLimitSteps has limited support)
+ Rename ConditionalBpRange -> BreakpointRange, cbp -> bpr, *conditional_bp_ranges -> *bp_ranges
Apr 28 2021
Apr 26 2021
Apr 23 2021
Apr 22 2021
+ Update Commands.md
I am seeing some false positives for sure (most of these artifital vars locs are being removed after some "constant propagation" passes). We definitely need to investigate how to remove these false positives here (for both, instr and var locations).
Apr 21 2021
Hi @djtodoro I am looking at this (slowly, sorry!).
Thanks both for the reviews.
+Remove unnecessary comment.
Apr 20 2021
Apr 19 2021
Apr 16 2021
Landing this tripped up some build bots (e.g. https://lab.llvm.org/buildbot/#/builders/109/builds/12664). I've created D100632 which should fix the issue iiuc.
Apr 15 2021
Apr 13 2021
Apr 12 2021
Apr 9 2021
You're preserving the location in the case that only one instruction is moved. Interestingly the linked document also says "a transformation should also preserve the debug location of an instruction that is moved between basic blocks, if the destination block already contains an instruction with an identical debug location." suggesting that you should only preserve the location if "an identical one" exists in the pred block. Scanning the block for an identical location sounds excessive. My gut reaction would be to simply add an additional check that the location of the instruction at the insertion point matches the (single) hoisted instruction's location before preserving the location, or alternatively always drop the location (i.e. remove the if guard altogether).
Apr 8 2021
+ Remove tbaa metadata from lit test.
+ Fix clang-tidy warning in unittest.
+ Move the code into a new function replaceDbgUsesOutsideBlock.
Apr 6 2021
LGTM with a few tiny nits, but please wait for a +1 from someone else too.
Mar 23 2021
I wonder if it would be better update Value::replaceUsesWithIf instead, which would reach more code. Though that shape of that change might need some discussion. For instance, would we pass in the MetadataAsValue-ValueAsMetadata-Value debug uses to the predicate, or alternatively, have a a separate predicate for debug uses? wdyt?
Mar 19 2021
Mar 18 2021
Mar 16 2021
FWIW the SelectionDAG fix LGTM. I'll leave the LSR part to someone more familiar with it.
Mar 15 2021
Thanks @teemperor. I've added a quick fix in 61d314024dc4 which fixes the immediate issue of passing in None to os.path.exists (it seems like loc.path being None is a valid state in dexter). I don't have a machine running macOS to test on so I'll keep my eye on the bot.
Mar 12 2021
Mar 11 2021
Hi @fhahn did you mean regression/lit test?
Mar 2 2021
Thank you @rnk.