[PR,tdep/17379] Fix internal-error when stack pointer is invalid
Commit Message
Pedro Alves wrote:
> See https://sourceware.org/bugzilla/show_bug.cgi?id=17384 .
>
> When safe_read_memory_integer call fails, GDB prints a
> surprising/confusing error message, more so in case the unwinder
> is triggered for some reason other than the "bt" command, like
> with "step"/"next". I take you're now seeing the same errors
> with this patch.
>
> IMO, printing the error is not something a low-level helper function
> like safe_read_memory_integer should be doing, as GDB uses it when
> probing with heuristics because it can't sure its guesses make sense
> (whether there's a frame at all, etc.) safe_frame_unwind_memory, which is
> used in rs6000_in_function_epilogue_p doesn't print the error either.
Agreed, it doesn't make sense for safe_read_memory_integer to ever
print an error. In fact, it doesn't make sense for it to start
using a routine that raises exceptions and then attempt to catch it.
The following patch simplifies the whole logic by just using
target_read_memory directly. Does this look reasonable?
[ B.t.w. the naming of safe_frame_unwind_memory is a bit weird. This
should either be "safe_read_memory" in corefile.c, or else something
like safe_get_frame_memory in analogy to get_frame_memory. ]
Tested on powerpc64le-linux.
Bye,
Ulrich
gdb/ChangeLog:
* corefile.c (struct captured_read_memory_integer_arguments): Remove.
(do_captured_read_memory_integer): Remove.
(safe_read_memory_integer): Use target_read_memory directly instead
of catching errors in do_captured_read_memory_integer.
Comments
On 09/17/2014 01:41 PM, Ulrich Weigand wrote:
> Pedro Alves wrote:
>
>> See https://sourceware.org/bugzilla/show_bug.cgi?id=17384 .
>>
>> When safe_read_memory_integer call fails, GDB prints a
>> surprising/confusing error message, more so in case the unwinder
>> is triggered for some reason other than the "bt" command, like
>> with "step"/"next". I take you're now seeing the same errors
>> with this patch.
>>
>> IMO, printing the error is not something a low-level helper function
>> like safe_read_memory_integer should be doing, as GDB uses it when
>> probing with heuristics because it can't sure its guesses make sense
>> (whether there's a frame at all, etc.) safe_frame_unwind_memory, which is
>> used in rs6000_in_function_epilogue_p doesn't print the error either.
>
> Agreed, it doesn't make sense for safe_read_memory_integer to ever
> print an error. In fact, it doesn't make sense for it to start
> using a routine that raises exceptions and then attempt to catch it.
> The following patch simplifies the whole logic by just using
> target_read_memory directly. Does this look reasonable?
Definitely reasonable. Looks great to me. Thanks for doing this.
>
> [ B.t.w. the naming of safe_frame_unwind_memory is a bit weird. This
> should either be "safe_read_memory" in corefile.c, or else something
> like safe_get_frame_memory in analogy to get_frame_memory. ]
Agreed. It seems like that and get_frame_memory were added
in order to make sure frame code consistently used
target_read_memory_nobpt to mask out breakpoints:
https://sourceware.org/ml/gdb-patches/2004-04/msg00067.html
Seems like all that wrapping is unnecessary nowadays, as we have to
go out of way to bypass breakpoint masking.
Thanks,
Pedro Alves
@@ -290,40 +290,6 @@ read_code (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
memory_error (status, memaddr);
}
-/* Argument / return result struct for use with
- do_captured_read_memory_integer(). MEMADDR and LEN are filled in
- by gdb_read_memory_integer(). RESULT is the contents that were
- successfully read from MEMADDR of length LEN. */
-
-struct captured_read_memory_integer_arguments
-{
- CORE_ADDR memaddr;
- int len;
- enum bfd_endian byte_order;
- LONGEST result;
-};
-
-/* Helper function for gdb_read_memory_integer(). DATA must be a
- pointer to a captured_read_memory_integer_arguments struct.
- Return 1 if successful. Note that the catch_errors() interface
- will return 0 if an error occurred while reading memory. This
- choice of return code is so that we can distinguish between
- success and failure. */
-
-static int
-do_captured_read_memory_integer (void *data)
-{
- struct captured_read_memory_integer_arguments *args
- = (struct captured_read_memory_integer_arguments*) data;
- CORE_ADDR memaddr = args->memaddr;
- int len = args->len;
- enum bfd_endian byte_order = args->byte_order;
-
- args->result = read_memory_integer (memaddr, len, byte_order);
-
- return 1;
-}
-
/* Read memory at MEMADDR of length LEN and put the contents in
RETURN_VALUE. Return 0 if MEMADDR couldn't be read and non-zero
if successful. */
@@ -333,19 +299,13 @@ safe_read_memory_integer (CORE_ADDR memaddr, int len,
enum bfd_endian byte_order,
LONGEST *return_value)
{
- int status;
- struct captured_read_memory_integer_arguments args;
-
- args.memaddr = memaddr;
- args.len = len;
- args.byte_order = byte_order;
+ gdb_byte buf[sizeof (LONGEST)];
- status = catch_errors (do_captured_read_memory_integer, &args,
- "", RETURN_MASK_ALL);
- if (status)
- *return_value = args.result;
+ if (target_read_memory (memaddr, buf, len))
+ return 0;
- return status;
+ *return_value = extract_signed_integer (buf, len, byte_order);
+ return 1;
}
LONGEST