[PATCHv2 1/2] gdb: prevent an assertion when computing the frame_id for an inline frame

[Pedro Alves]

Hi Andrew,

I've read this, and I think you came up with a reasonable solution.

Some minor-ish comments below.

On 2021-06-21 8:46 p.m., Andrew Burgess wrote:

> +static void foo (void);
> +static void bar (void);
> +
> +volatile int global_var;
> +volatile int level_counter;
> +
> +static void __attribute__((noinline))
> +bar (void)
> +{
> +  /* Do some work.  */
> +  ++global_var;
> +
> +  /* Now the inline function.  */
> +  --level_counter;
> +  foo ();
> +  ++level_counter;
> +
> +  /* Do some work.  */
> +  ++global_var;
> +}
> +
> +static inline void __attribute__((__always_inline__))
> +foo (void)
> +{
> +  if (level_counter > 1)
> +    {
> +      --level_counter;
> +      bar ();
> +      ++level_counter;
> +    }
> +  else
> +    ++global_var;	/* Break here.  */
> +}

I'd suggest renaming these "foo" "bar" functions to "normal_func" "inline_func"
or "norm" "inln" or something like that.  I think it'll make the backtraces in
the .exp code more obvious.

> +
> +int
> +main ()
> +{
> +  level_counter = 6;
> +  bar ();
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/inline-frame-bad-unwind.exp b/gdb/testsuite/gdb.base/inline-frame-bad-unwind.exp
> new file mode 100644
> index 00000000000..49c35517801
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/inline-frame-bad-unwind.exp

I'd suggest naming this something with "cycle" instead of "bad" as being more
to the point.  There can be many forms of badness.

> +
> +# This test checks for an edge case when unwinding inline frames which
> +# occur towards the older end of the stack when the stack ends with a
> +# cycle.  Consider this well formed stack:
> +#
> +#   main -> normal_frame -> inline_frame
> +#
> +# Now consider that, for whatever reason, the stack unwinding of
> +# "normal_frame" becomes corrupted, such that the stack appears to be
> +# this:
> +#
> +#   .-> normal_frame -> inline_frame
> +#   |      |
> +#   '------'
> +#
> +# When confrontend with such a situation we would expect GDB to detect

Typo: confrontend -> confronted

> +
> +# Check the unbroken stack.
> +gdb_test_sequence "bt"  "Backtrace when the unwind is left unbroken" {
> +    "\\r\\n#0 \[^\r\n\]* foo \\(\\) at "
> +    "\\r\\n#1 \[^\r\n\]* bar \\(\\) at "
> +    "\\r\\n#2 \[^\r\n\]* foo \\(\\) at "
> +    "\\r\\n#3 \[^\r\n\]* bar \\(\\) at "
> +    "\\r\\n#4 \[^\r\n\]* foo \\(\\) at "
> +    "\\r\\n#5 \[^\r\n\]* bar \\(\\) at "
> +    "\\r\\n#6 \[^\r\n\]* main \\(\\) at "
> +}
> +
> +# Arrange to introduce a stack cycle at frame 5.
> +gdb_test_no_output "python stop_at_level=5"
> +gdb_test "maint flush register-cache" \
> +    "Register cache flushed\\." ""

How about using with_test_prefix instead of suppressing the test message?
Like:

with_test_prefix "broken at frame 5 {
  # Arrange to introduce a stack cycle at frame 5.
  gdb_test_no_output "python stop_at_level=5"
  gdb_test "maint flush register-cache" "Register cache flushed\\."
  gdb_test_sequence "bt" "" {
    ...
  }
}

> +gdb_test_sequence "bt"  "Backtrace when the unwind is broken at frame 5" {

Spurious double space, and lowercase "Backtrace".

> +    "\\r\\n#0 \[^\r\n\]* foo \\(\\) at "
> +    "\\r\\n#1 \[^\r\n\]* bar \\(\\) at "
> +    "\\r\\n#2 \[^\r\n\]* foo \\(\\) at "
> +    "\\r\\n#3 \[^\r\n\]* bar \\(\\) at "
> +    "\\r\\n#4 \[^\r\n\]* foo \\(\\) at "
> +    "\\r\\n#5 \[^\r\n\]* bar \\(\\) at "
> +    "\\r\\nBacktrace stopped: previous frame identical to this frame \\(corrupt stack\\?\\)"

Actually, I don't think gdb_test_sequence is the right proc for these tests, because it
consumes lines between each pattern.  I mean, above, the test will pass if GDB prints
frame #6 too, before printing the "Backtrace stopped" line.  E.g., with this change,
the test will still pass:

@@ -92,10 +90,6 @@ gdb_test "maint flush register-cache" \
 gdb_test_sequence "bt"  "Backtrace when the unwind is broken at frame 5" {
     "\\r\\n#0 \[^\r\n\]* foo \\(\\) at "
     "\\r\\n#1 \[^\r\n\]* bar \\(\\) at "
-    "\\r\\n#2 \[^\r\n\]* foo \\(\\) at "
-    "\\r\\n#3 \[^\r\n\]* bar \\(\\) at "
-    "\\r\\n#4 \[^\r\n\]* foo \\(\\) at "
-    "\\r\\n#5 \[^\r\n\]* bar \\(\\) at "
     "\\r\\nBacktrace stopped: previous frame identical to this frame \\(corrupt stack\\?\\)"
 }

> diff --git a/gdb/testsuite/gdb.base/inline-frame-bad-unwind.py b/gdb/testsuite/gdb.base/inline-frame-bad-unwind.py
> new file mode 100644
> index 00000000000..370f86cc610
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/inline-frame-bad-unwind.py
> @@ -0,0 +1,85 @@
> +# Copyright (C) 2021 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +import gdb
> +from gdb.unwinder import Unwinder
> +
> +# Set this to the stack level the backtrace should be corrupted at.
> +# This will only work for frame 1, 3, or 5 in the test this unwinder
> +# was written for.
> +stop_at_level = None
> +
> +# Set this to the stack frame size of frames 1, 3, and 5.  These
> +# frames wil all have the same stack frame size as they are the same

Typo "wil" -> "will".