Commit Message
On 10/28/2015 07:38 PM, Pedro Alves wrote:
> On 10/28/2015 07:36 PM, Pedro Alves wrote:
>> On 10/28/2015 07:29 PM, Simon Marchi wrote:
>>
>>> The status comes from gdbscm_disasm_read_memory returning TARGET_XFER_E_IO:
>>>
>>> return status != NULL ? TARGET_XFER_E_IO : 0;
>>>
>>> Does it make sense that this function returns TARGET_XFER_E_IO, and
>>> not just -1 (or any other non-zero value) on error? It's an
>>> all-or-nothing memory read function, unlike those of the xfer_partial
>>> interface.
>>>
>>> I would have done a change similar to what you have done in
>>> target_read_memory&co: make gdbscm_disasm_read_memory return -1 on
>>> error, and change
>>> memory_error (status, memaddr);
>>> to
>>> memory_error (TARGET_XFER_E_IO, memaddr);
>>>
>>> Would it make sense?
>>
>> I had the same thoughts when I did the target_read_memory&co patch,
>> and went through all the memory_error callers. In the end I left
>> it be because of the IWBN comment:
>>
>> /* TODO: IWBN to distinguish problems reading target memory versus problems
>> with the port (e.g., EOF).
>> We return TARGET_XFER_E_IO here as that's what memory_error looks for. */
>> return status != NULL ? TARGET_XFER_E_IO : 0;
>>
>> Either way is fine with me. Doug, what would you prefer?
>>
>> Cast?
>> Hardcode TARGET_XFER_E_IO in the memory_error call?
>> Other?
>
> Hmm, reading the comment back, I actually agree with Simon.
> The comment refers to distinguishing memory errors from something
> else not memory errors. In that "something else" case, sounds like
> we wouldn't end up calling memory_error at all. So sounds like Simon's
> suggestion would be the clearer way to go. WDYT?
Like this?
From: Pedro Alves <palves@redhat.com>
Date: 2015-10-27 17:25:12 +0000
guile disassembly hardcode TARGET_XFER_E_IO
Instead of adding a cast at the memory_error call, as needed for C++,
and have the reader understand the indirection, make it simple and
hardcode the generic memory error at the memory_error call site.
gdb/ChangeLog:
2015-10-28 Pedro Alves <palves@redhat.com>
* guile/scm-disasm.c (gdbscm_disasm_read_memory): Return -1 on
error instead of TARGET_XFER_E_IO.
(gdbscm_disasm_memory_error): Always pass TARGET_XFER_E_IO to
memory_error.
---
gdb/guile/scm-disasm.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
Comments
On 10/28/2015 07:46 PM, Pedro Alves wrote:
> On 10/28/2015 07:38 PM, Pedro Alves wrote:
>> On 10/28/2015 07:36 PM, Pedro Alves wrote:
>>> On 10/28/2015 07:29 PM, Simon Marchi wrote:
>>>
>>>> The status comes from gdbscm_disasm_read_memory returning TARGET_XFER_E_IO:
>>>>
>>>> return status != NULL ? TARGET_XFER_E_IO : 0;
>>>>
>>>> Does it make sense that this function returns TARGET_XFER_E_IO, and
>>>> not just -1 (or any other non-zero value) on error? It's an
>>>> all-or-nothing memory read function, unlike those of the xfer_partial
>>>> interface.
>>>>
>>>> I would have done a change similar to what you have done in
>>>> target_read_memory&co: make gdbscm_disasm_read_memory return -1 on
>>>> error, and change
>>>> memory_error (status, memaddr);
>>>> to
>>>> memory_error (TARGET_XFER_E_IO, memaddr);
>>>>
>>>> Would it make sense?
>>>
>>> I had the same thoughts when I did the target_read_memory&co patch,
>>> and went through all the memory_error callers. In the end I left
>>> it be because of the IWBN comment:
>>>
>>> /* TODO: IWBN to distinguish problems reading target memory versus problems
>>> with the port (e.g., EOF).
>>> We return TARGET_XFER_E_IO here as that's what memory_error looks for. */
>>> return status != NULL ? TARGET_XFER_E_IO : 0;
>>>
>>> Either way is fine with me. Doug, what would you prefer?
>>>
>>> Cast?
>>> Hardcode TARGET_XFER_E_IO in the memory_error call?
>>> Other?
>>
>> Hmm, reading the comment back, I actually agree with Simon.
>> The comment refers to distinguishing memory errors from something
>> else not memory errors. In that "something else" case, sounds like
>> we wouldn't end up calling memory_error at all. So sounds like Simon's
>> suggestion would be the clearer way to go. WDYT?
>
> Like this?
>
> From: Pedro Alves <palves@redhat.com>
> Date: 2015-10-27 17:25:12 +0000
>
> guile disassembly hardcode TARGET_XFER_E_IO
>
> Instead of adding a cast at the memory_error call, as needed for C++,
> and have the reader understand the indirection, make it simple and
> hardcode the generic memory error at the memory_error call site.
>
> gdb/ChangeLog:
> 2015-10-28 Pedro Alves <palves@redhat.com>
>
> * guile/scm-disasm.c (gdbscm_disasm_read_memory): Return -1 on
> error instead of TARGET_XFER_E_IO.
> (gdbscm_disasm_memory_error): Always pass TARGET_XFER_E_IO to
> memory_error.
I pushed this one in now.
Thanks,
Pedro Alves
@@ -119,9 +119,8 @@ gdbscm_disasm_read_memory (bfd_vma memaddr, bfd_byte *myaddr,
status = gdbscm_with_guile (gdbscm_disasm_read_memory_worker, &data);
/* TODO: IWBN to distinguish problems reading target memory versus problems
- with the port (e.g., EOF).
- We return TARGET_XFER_E_IO here as that's what memory_error looks for. */
- return status != NULL ? TARGET_XFER_E_IO : 0;
+ with the port (e.g., EOF). */
+ return status != NULL ? -1 : 0;
}
/* disassemble_info.memory_error_func for gdbscm_print_insn_from_port.
@@ -133,7 +132,7 @@ static void
gdbscm_disasm_memory_error (int status, bfd_vma memaddr,
struct disassemble_info *info)
{
- memory_error (status, memaddr);
+ memory_error (TARGET_XFER_E_IO, memaddr);
}
/* disassemble_info.print_address_func for gdbscm_print_insn_from_port.