This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][Dexter] Unreachable line stepped onto after SimplifyCFG.
AbandonedPublic

Authored by CarlosAlbertoEnciso on Oct 4 2018, 7:09 AM.

Details

Summary

For the below test:

int main() {
  volatile int foo = 0;
  int read1 = foo;
  int brains = foo;

  if (read1 > 3) {
    brains *= 2;
    brains += 1;
  }

  return brains;
}

When in the debugger, on the line "if (read1 > 3)", and we step from the
'if' condition, onto the addition, then back to the 'if' again, which is
misleading, as described by:

PR38762

Diff Detail

Repository
rL LLVM

Event Timeline

The code looks fine, I have some comments about the comments.

lib/Transforms/Utils/Local.cpp
2535 ↗(On Diff #168272)

Please also explain why if possible. I think the reason is different for DILoctions and dbg.values.

2538 ↗(On Diff #168272)
TODO: Extend llvm.dbg.value to take more than one SSA Value (PR39141) to encode predicated DIExpressions that yield different results on different code paths.
2540 ↗(On Diff #168272)

this sentence is garbled :-)

If I parsed the sentence correctly it sounds like a non-sequitur to me: having conditional dbg.values won't affect our options for DILocations at all. Or are you using "location" in the .debug_loc sense? That's confusing, let's use dbg.value instead.

lib/Transforms/Utils/SimplifyCFG.cpp
2373 ↗(On Diff #168272)

please clang-format.

test/CodeGen/X86/pr38762.ll
108 ↗(On Diff #168272)

Perhaps compile with -gno-column-info, so the test input isn't quite as long?

vsk added inline comments.Oct 4 2018, 7:16 PM
include/llvm/Transforms/Utils/Local.h
454 ↗(On Diff #168272)

I think function name and comments here could be clearer.

The action seems closer to 'hoisting' than 'promoting'. And the function is hoisting all of the instructions in the IfBlock, not just the phis. How about: 'hoistAllInstructionsInto'?

lib/Transforms/Utils/Local.cpp
2535 ↗(On Diff #168272)

Right. Moving locations around degrades single-stepping and profiling. Moving dbg.values around makes variable updates appear re-ordered in a debug session.

CarlosAlbertoEnciso marked 7 inline comments as done.Oct 5 2018, 5:11 AM
CarlosAlbertoEnciso added inline comments.
include/llvm/Transforms/Utils/Local.h
454 ↗(On Diff #168272)

Taking your suggested function name hoistAllInstructionsInto . Is a better description for what the function does. I have made some changes to the comments, hoping for more clarity.

lib/Transforms/Utils/Local.cpp
2535 ↗(On Diff #168272)

Updated the comments to include a better explanation about the actions taken on DILocations and dbg.values.

2535 ↗(On Diff #168272)

Taking your feedback, as the base for the updated comments.

2538 ↗(On Diff #168272)

Included your description about the TODO work.

2540 ↗(On Diff #168272)

this sentence is garbled :-)

Now reading my description, I realized that something was missing and it does not make much sense :-)

I have updated the comments and made a clear distinction between debug locations (DILocations) and debug.values.

lib/Transforms/Utils/SimplifyCFG.cpp
2373 ↗(On Diff #168272)

Done.

test/CodeGen/X86/pr38762.ll
108 ↗(On Diff #168272)

Used your suggested option -gno-column-info.

CarlosAlbertoEnciso marked 6 inline comments as done.

Address issues raised by the reviewers.

Improve the comments in order to add more clarity.
Reduce test case by using no-column-info option.
Better description and name for the new function.

Thanks for the comments and suggestions.

CarlosAlbertoEnciso marked 7 inline comments as done.Oct 5 2018, 5:16 AM

The code looks fine, I have some comments about the comments.

lib/Transforms/Utils/Local.cpp
2535 ↗(On Diff #168272)

Just to be clear: When I said "please explain" I meant: in the comment in the code, not to me in the review :-)

aprantl accepted this revision.Oct 5 2018, 9:07 AM

LGTM with outstanding comments addressed. Thanks!

test/CodeGen/X86/pr38762.ll
51 ↗(On Diff #168456)

Do we need all the !tbaa metadata for the test?

This revision is now accepted and ready to land.Oct 5 2018, 9:07 AM
CarlosAlbertoEnciso marked 2 inline comments as done.Oct 8 2018, 7:18 AM
CarlosAlbertoEnciso added inline comments.
lib/Transforms/Utils/Local.cpp
2535 ↗(On Diff #168272)

I have added extra comments in the code for the function 'hoistAllInstructionsInto'.

test/CodeGen/X86/pr38762.ll
51 ↗(On Diff #168456)

Good point. I have removed all the !tbaa metadata. It is not required.

CarlosAlbertoEnciso marked 2 inline comments as done.

Address issues raised by the reviewers.

@vsk: Do you have any additional comments.

Thanks to the reviewers for their comments and suggestions.

vsk added a comment.Oct 8 2018, 11:53 AM

The logic looks fine, but the comment in hoistAllInstructionsInto should be shorter. One or two sentences would be ideal. Readers can look at the test case for more context. Thanks!

Address issues raised by the reviewers.

Improve the comments in order to add more clarity.

Thanks for the comments and suggestions.

vsk accepted this revision.Oct 9 2018, 2:27 PM

Thank you, lgtm.

Thanks very much to all the reviewers for your valuable comments and suggestions.

This revision was automatically updated to reflect the committed changes.

This broke compilation for me, compiling some source files now fail due to triggered assertions, see PR39243 for details. I guess I'll revert this in a while unless there's someone to fix it soon.

This broke compilation for me, compiling some source files now fail due to triggered assertions, see PR39243 for details. I guess I'll revert this in a while unless there's someone to fix it soon.

@mstorsjo, I am sorry about the issues caused by the commit. I am in the process of reverting the changes. Thanks.

@mstorsjo, The change has been reverted. Thanks for creating the bugzilla.

The patch caused some assertions. For more details see PR39243.

This revision is now accepted and ready to land.Oct 11 2018, 11:51 AM

This revision caused builbot failures.

A new review have been created to address both issues D53287.