Page MenuHomePhabricator

[Local] Add dbg location on unreachable inst in changeToUnreachable
ClosedPublic

Authored by gramanas on Aug 6 2018, 10:24 AM.

Details

Summary

As show in https://bugs.llvm.org/show_bug.cgi?id=37960
it would be desirable to have debug location in the unreachable
instruction.

If the unreachable is lowered to llvm.trap() or similar,
it could be really useful to have line/scope information there
to make forensic debugging more pleasant.

I am not sure whether to add a unit test for this funtion.

Diff Detail

Repository
rL LLVM

Event Timeline

gramanas created this revision.Aug 6 2018, 10:24 AM
vsk added a comment.Aug 6 2018, 10:50 AM

Thanks for doing this! Yes, please add a unit test in unittests/Transforms/Utils/Local.cpp.

gramanas updated this revision to Diff 159561.Aug 7 2018, 11:53 AM

Add unit test.

vsk added inline comments.Aug 7 2018, 12:01 PM
unittests/Transforms/Utils/Local.cpp
616 ↗(On Diff #159561)

Why not just .front()?

620 ↗(On Diff #159561)

nit, 1U is easier to read.

620 ↗(On Diff #159561)

Please add a comment when using boolean literals as parameters. Instead of 'false', it's clearer to write /*UseLLVMTrap*/false.

626 ↗(On Diff #159561)

This needs to check that the correct location is present, not just that the instruction has metadata.

gramanas updated this revision to Diff 159572.Aug 7 2018, 12:29 PM
gramanas marked 4 inline comments as done.
  • update according to comments
vsk accepted this revision.Aug 7 2018, 12:31 PM

LGTM, thanks!

unittests/Transforms/Utils/Local.cpp
627 ↗(On Diff #159572)

Nit, this assertion is now redundant.

This revision is now accepted and ready to land.Aug 7 2018, 12:31 PM
This revision was automatically updated to reflect the committed changes.
gramanas marked an inline comment as done.