[PATCH] guile/: Add enum casts

[PATCH] guile/: Add enum casts

[Pedro Alves]

In both cases the casts looks appropriate to me.  In the
gdbscm_disasm_memory_error case, the status is marshaled through the
opcodes disassemble interface.  In the
gdbscm_unwind_stop_reason_string case, the int comes from Guile.

gdb/ChangeLog:
2015-10-28  Pedro Alves  <palves@redhat.com>

	* guile/scm-disasm.c (gdbscm_disasm_memory_error): Add cast.
	* guile/scm-frame.c (gdbscm_unwind_stop_reason_string): Add cast.
---
 gdb/guile/scm-disasm.c | 2 +-
 gdb/guile/scm-frame.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/guile/scm-disasm.c b/gdb/guile/scm-disasm.c
index 78b38df..0cc2f84 100644
--- a/gdb/guile/scm-disasm.c
+++ b/gdb/guile/scm-disasm.c
@@ -133,7 +133,7 @@ static void
 gdbscm_disasm_memory_error (int status, bfd_vma memaddr,
 			    struct disassemble_info *info)
 {
-  memory_error (status, memaddr);
+  memory_error ((enum target_xfer_status) status, memaddr);
 }
 
 /* disassemble_info.print_address_func for gdbscm_print_insn_from_port.
diff --git a/gdb/guile/scm-frame.c b/gdb/guile/scm-frame.c
index 24e26e8..55e0faf 100644
--- a/gdb/guile/scm-frame.c
+++ b/gdb/guile/scm-frame.c
@@ -1045,7 +1045,7 @@ gdbscm_unwind_stop_reason_string (SCM reason_scm)
   if (reason < UNWIND_FIRST || reason > UNWIND_LAST)
     scm_out_of_range (FUNC_NAME, reason_scm);
 
-  str = unwind_stop_reason_to_string (reason);
+  str = unwind_stop_reason_to_string ((enum unwind_stop_reason) reason);
   return gdbscm_scm_from_c_string (str);
 }
 \f
-- 
1.9.3

Re: [PATCH] guile/: Add enum casts

[Simon Marchi]

On 28 October 2015 at 14:54, Pedro Alves <palves@redhat.com> wrote:
> In both cases the casts looks appropriate to me.  In the
> gdbscm_disasm_memory_error case, the status is marshaled through the
> opcodes disassemble interface.  In the
> gdbscm_unwind_stop_reason_string case, the int comes from Guile.
>
> gdb/ChangeLog:
> 2015-10-28  Pedro Alves  <palves@redhat.com>
>
>         * guile/scm-disasm.c (gdbscm_disasm_memory_error): Add cast.
>         * guile/scm-frame.c (gdbscm_unwind_stop_reason_string): Add cast.
> ---
>  gdb/guile/scm-disasm.c | 2 +-
>  gdb/guile/scm-frame.c  | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/guile/scm-disasm.c b/gdb/guile/scm-disasm.c
> index 78b38df..0cc2f84 100644
> --- a/gdb/guile/scm-disasm.c
> +++ b/gdb/guile/scm-disasm.c
> @@ -133,7 +133,7 @@ static void
>  gdbscm_disasm_memory_error (int status, bfd_vma memaddr,
>                             struct disassemble_info *info)
>  {
> -  memory_error (status, memaddr);
> +  memory_error ((enum target_xfer_status) status, memaddr);
>  }
>
>  /* disassemble_info.print_address_func for gdbscm_print_insn_from_port.
> diff --git a/gdb/guile/scm-frame.c b/gdb/guile/scm-frame.c
> index 24e26e8..55e0faf 100644
> --- a/gdb/guile/scm-frame.c
> +++ b/gdb/guile/scm-frame.c
> @@ -1045,7 +1045,7 @@ gdbscm_unwind_stop_reason_string (SCM reason_scm)
>    if (reason < UNWIND_FIRST || reason > UNWIND_LAST)
>      scm_out_of_range (FUNC_NAME, reason_scm);
>
> -  str = unwind_stop_reason_to_string (reason);
> +  str = unwind_stop_reason_to_string ((enum unwind_stop_reason) reason);
>    return gdbscm_scm_from_c_string (str);
>  }
>
> --
> 1.9.3
>

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?

Re: [PATCH] guile/: Add enum casts

[Pedro Alves]

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?

Thanks,
Pedro Alves

Re: [PATCH] guile/: Add enum casts

[Pedro Alves]

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?

Thanks,
Pedro Alves

Re: [PATCH] guile/: Add enum casts

[Pedro Alves]

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(-)

diff --git a/gdb/guile/scm-disasm.c b/gdb/guile/scm-disasm.c
index 78b38df..c9e940d 100644
--- a/gdb/guile/scm-disasm.c
+++ b/gdb/guile/scm-disasm.c
@@ -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.

Re: [PATCH] guile/: Add enum casts

[Pedro Alves]

On 10/28/2015 06:54 PM, Pedro Alves wrote:
> In both cases the casts looks appropriate to me.  In the
> gdbscm_disasm_memory_error case, the status is marshaled through the
> opcodes disassemble interface.  In the
> gdbscm_unwind_stop_reason_string case, the int comes from Guile.
> 
> gdb/ChangeLog:
> 2015-10-28  Pedro Alves  <palves@redhat.com>
> 
> 	* guile/scm-disasm.c (gdbscm_disasm_memory_error): Add cast.
> 	* guile/scm-frame.c (gdbscm_unwind_stop_reason_string): Add cast.

I split out the second hunk and pushed it in as an obvious change.

--------
From: Pedro Alves <palves@redhat.com>
Subject: [PATCH] guile/: Add enum cast

gdb/ChangeLog:
2015-10-29  Pedro Alves  <palves@redhat.com>

	* guile/scm-frame.c (gdbscm_unwind_stop_reason_string): Add cast.
---
 gdb/ChangeLog         | 4 ++++
 gdb/guile/scm-frame.c | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 000566e..9433221 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2015-10-29  Pedro Alves  <palves@redhat.com>
+
+	* guile/scm-frame.c (gdbscm_unwind_stop_reason_string): Add cast.
+
 2015-10-29  Eli Zaretskii  <eliz@gnu.org>
 
 	* utils.c (init_page_info): Disable paging if INSIDE_EMACS is set
diff --git a/gdb/guile/scm-frame.c b/gdb/guile/scm-frame.c
index 24e26e8..55e0faf 100644
--- a/gdb/guile/scm-frame.c
+++ b/gdb/guile/scm-frame.c
@@ -1045,7 +1045,7 @@ gdbscm_unwind_stop_reason_string (SCM reason_scm)
   if (reason < UNWIND_FIRST || reason > UNWIND_LAST)
     scm_out_of_range (FUNC_NAME, reason_scm);
 
-  str = unwind_stop_reason_to_string (reason);
+  str = unwind_stop_reason_to_string ((enum unwind_stop_reason) reason);
   return gdbscm_scm_from_c_string (str);
 }
 \f
-- 
1.9.3

Re: [PATCH] guile/: Add enum casts

[Pedro Alves]

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