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

Pedro Alves
2021-07-05T11:39:48Z

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".