[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

[David Blaikie via Phabricator via lldb-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

In D91734#2480474 <https://reviews.llvm.org/D91734#2480474>, @probinson wrote:

> This version of the patch avoids the weirdness I was seeing with prolog instructions in certain cases.
>
> The gdb test suite is largely happy with this; it avoids the new failures I mentioned previously, and one test needed to have some "next" commands updated.  Here's the gdb test suite patch (against 8.3; I can provide more context if it's not clear how to apply to later versions).
>
>   diff --git a/gdb-8.3/gdb/testsuite/gdb.base/foll-exec.exp b/gdb-8.3/gdb/testsuite/gdb.base/foll-exec.exp
>   index 3e0653a1..61ee752f 100644
>   --- a/gdb-8.3/gdb/testsuite/gdb.base/foll-exec.exp
>   +++ b/gdb-8.3/gdb/testsuite/gdb.base/foll-exec.exp
>   @@ -115,7 +115,10 @@ proc do_exec_tests {} {
>       # We should stop in execd-program, at its first statement.
>       #
>       set execd_line [gdb_get_line_number "after-exec" $srcfile2]
>   -   send_gdb "next\n"
>   +    # Clang will emit source locations for the parameter evaluation.
>   +    # Ideally we'd "next" until we saw 'execlp' again in the source display,
>   +    # then do one more.
>   +   send_gdb "next 3\n"
>       gdb_expect {
>         -re ".*xecuting new program: .*${testfile2}.*${srcfile2}:${execd_line}.*int  local_j = argc;.*$gdb_prompt $"\
>                         {pass "step through execlp call"}
>   @@ -263,7 +266,7 @@ proc do_exec_tests {} {
>       # the newly-exec'd program, not after the remaining step-count
>       # reaches zero.
>       #
>   -   send_gdb "next 2\n"
>   +   send_gdb "next 3\n"
>       gdb_expect {
>         -re ".*xecuting new program: .*${testfile2}.*${srcfile2}:${execd_line}.*int  local_j = argc;.*$gdb_prompt $"\
>                         {pass "step through execl call"}
>
> I think I might want to add one more lit test, because the LLVM suite didn't catch a thinko that was the root cause of that last prolog weirdness.  But this version is totally ready for review.

Thanks - that seems to cover Google's internal gdb test execution as well. Appreciate the test patch!

I haven't looked closely, but I take it this one test modification seems reasonable to you? I guess we're stepping into some subexpressions on a function call that we previously didn't? (they didn't have any instructions attributed to them until now, but it's not unreasonable that they have them attributed - for materializing constants to pass to a function call when the function call/constants are split over multiple lines, etc)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91734/new/

https://reviews.llvm.org/D91734