* [RFC][PATCH v2][PR remote/16896] Invalidate a register in cache when a remote target failed to write it.
@ 2014-05-22 14:23 Pierre Langlois
2014-06-02 9:12 ` [PING][RFC][PATCH " Pierre Langlois
2014-06-12 13:23 ` [RFC][PATCH " Pedro Alves
0 siblings, 2 replies; 6+ messages in thread
From: Pierre Langlois @ 2014-05-22 14:23 UTC (permalink / raw)
To: gdb-patches, palves
Hello all,
This patch addresses the issue in patch [1] generically as pointed out by
a comment [2].
As opposed to invalidating a register in cache in the remote target before
throwing an error, we can do this using a cleanup in regcache_raw_write. This
patch adds routines to add a regcache_invalidate cleanup to the current chain.
We can use this before target_store_registers and discard it after if an error
was not thrown.
[1] https://sourceware.org/ml/gdb-patches/2014-05/msg00083.html
[2] https://sourceware.org/ml/gdb-patches/2014-05/msg00357.html
2014-05-20 Pierre Langlois <pierre.langlois@embecosm.com>
PR remote/16896
* regcache.c (register_to_invalidate): New structure. Combines a pointer
to a struct regcache and a register number.
(do_register_invalidate): New function. Call regcache_invalidate.
(make_cleanup_regcache_invalidate): New function. Construct a cleanup
for invalidating a register.
(regcache_raw_write): Call make_cleanup_regcache_invalidate
* regcache.h (make_cleanup_regcache_invalidate): New prototype.
---
gdb/regcache.c | 41 ++++++++++++++++++++++++++++++++++++++---
gdb/regcache.h | 2 ++
2 files changed, 40 insertions(+), 3 deletions(-)
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 8b588c6..87a6b02 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -267,6 +267,30 @@ make_cleanup_regcache_xfree (struct regcache *regcache)
return make_cleanup (do_regcache_xfree, regcache);
}
+/* Cleanup routines for invalidating a register. */
+
+struct register_to_invalidate
+{
+ struct regcache *regcache;
+ int regnum;
+};
+
+static void
+do_regcache_invalidate (void *data)
+{
+ struct register_to_invalidate *reg = data;
+ regcache_invalidate (reg->regcache, reg->regnum);
+}
+
+struct cleanup *
+make_cleanup_regcache_invalidate (struct regcache *regcache, int regnum)
+{
+ struct register_to_invalidate* reg = XNEW (struct register_to_invalidate);
+ reg->regcache = regcache;
+ reg->regnum = regnum;
+ return make_cleanup_dtor (do_regcache_invalidate, (void *) reg, xfree);
+}
+
/* Return REGCACHE's architecture. */
struct gdbarch *
@@ -846,7 +870,8 @@ void
regcache_raw_write (struct regcache *regcache, int regnum,
const gdb_byte *buf)
{
- struct cleanup *old_chain;
+ struct cleanup *chain_before_save_inferior;
+ struct cleanup *chain_before_invalidate_register;
gdb_assert (regcache != NULL && buf != NULL);
gdb_assert (regnum >= 0 && regnum < regcache->descr->nr_raw_registers);
@@ -864,16 +889,26 @@ regcache_raw_write (struct regcache *regcache, int regnum,
regcache->descr->sizeof_register[regnum]) == 0))
return;
- old_chain = save_inferior_ptid ();
+ chain_before_save_inferior = save_inferior_ptid ();
inferior_ptid = regcache->ptid;
target_prepare_to_store (regcache);
memcpy (register_buffer (regcache, regnum), buf,
regcache->descr->sizeof_register[regnum]);
regcache->register_status[regnum] = REG_VALID;
+
+ /* Register a cleanup function for invalidating the register after it is
+ written, in case of a failure. */
+ chain_before_invalidate_register =
+ make_cleanup_regcache_invalidate (regcache, regnum);
+
target_store_registers (regcache, regnum);
- do_cleanups (old_chain);
+ /* The target did not throw an error so we can discard invalidating the
+ register and restore the cleanup chain to what it was. */
+ discard_cleanups (chain_before_invalidate_register);
+
+ do_cleanups (chain_before_save_inferior);
}
void
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 8423f57..bb40b65 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -33,6 +33,8 @@ extern struct regcache *get_thread_arch_aspace_regcache (ptid_t,
void regcache_xfree (struct regcache *regcache);
struct cleanup *make_cleanup_regcache_xfree (struct regcache *regcache);
+struct cleanup *make_cleanup_regcache_invalidate (struct regcache *regcache,
+ int regnum);
struct regcache *regcache_xmalloc (struct gdbarch *gdbarch,
struct address_space *aspace);
--
1.9.0
^ permalink raw reply [flat|nested] 6+ messages in thread* [PING][RFC][PATCH v2][PR remote/16896] Invalidate a register in cache when a remote target failed to write it.
2014-05-22 14:23 [RFC][PATCH v2][PR remote/16896] Invalidate a register in cache when a remote target failed to write it Pierre Langlois
@ 2014-06-02 9:12 ` Pierre Langlois
2014-06-11 16:58 ` [PING^2][RFC][PATCH " Pierre Langlois
2014-06-12 13:23 ` [RFC][PATCH " Pedro Alves
1 sibling, 1 reply; 6+ messages in thread
From: Pierre Langlois @ 2014-06-02 9:12 UTC (permalink / raw)
To: gdb-patches, palves
Ping.
On 22/05/14 15:23, Pierre Langlois wrote:
> Hello all,
>
> This patch addresses the issue in patch [1] generically as pointed out by
> a comment [2].
>
> As opposed to invalidating a register in cache in the remote target before
> throwing an error, we can do this using a cleanup in regcache_raw_write. This
> patch adds routines to add a regcache_invalidate cleanup to the current chain.
> We can use this before target_store_registers and discard it after if an error
> was not thrown.
>
> [1] https://sourceware.org/ml/gdb-patches/2014-05/msg00083.html
> [2] https://sourceware.org/ml/gdb-patches/2014-05/msg00357.html
>
> 2014-05-20 Pierre Langlois <pierre.langlois@embecosm.com>
>
> PR remote/16896
> * regcache.c (register_to_invalidate): New structure. Combines a pointer
> to a struct regcache and a register number.
> (do_register_invalidate): New function. Call regcache_invalidate.
> (make_cleanup_regcache_invalidate): New function. Construct a cleanup
> for invalidating a register.
> (regcache_raw_write): Call make_cleanup_regcache_invalidate
> * regcache.h (make_cleanup_regcache_invalidate): New prototype.
>
> ---
> gdb/regcache.c | 41 ++++++++++++++++++++++++++++++++++++++---
> gdb/regcache.h | 2 ++
> 2 files changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 8b588c6..87a6b02 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -267,6 +267,30 @@ make_cleanup_regcache_xfree (struct regcache *regcache)
> return make_cleanup (do_regcache_xfree, regcache);
> }
>
> +/* Cleanup routines for invalidating a register. */
> +
> +struct register_to_invalidate
> +{
> + struct regcache *regcache;
> + int regnum;
> +};
> +
> +static void
> +do_regcache_invalidate (void *data)
> +{
> + struct register_to_invalidate *reg = data;
> + regcache_invalidate (reg->regcache, reg->regnum);
> +}
> +
> +struct cleanup *
> +make_cleanup_regcache_invalidate (struct regcache *regcache, int regnum)
> +{
> + struct register_to_invalidate* reg = XNEW (struct register_to_invalidate);
> + reg->regcache = regcache;
> + reg->regnum = regnum;
> + return make_cleanup_dtor (do_regcache_invalidate, (void *) reg, xfree);
> +}
> +
> /* Return REGCACHE's architecture. */
>
> struct gdbarch *
> @@ -846,7 +870,8 @@ void
> regcache_raw_write (struct regcache *regcache, int regnum,
> const gdb_byte *buf)
> {
> - struct cleanup *old_chain;
> + struct cleanup *chain_before_save_inferior;
> + struct cleanup *chain_before_invalidate_register;
>
> gdb_assert (regcache != NULL && buf != NULL);
> gdb_assert (regnum >= 0 && regnum < regcache->descr->nr_raw_registers);
> @@ -864,16 +889,26 @@ regcache_raw_write (struct regcache *regcache, int regnum,
> regcache->descr->sizeof_register[regnum]) == 0))
> return;
>
> - old_chain = save_inferior_ptid ();
> + chain_before_save_inferior = save_inferior_ptid ();
> inferior_ptid = regcache->ptid;
>
> target_prepare_to_store (regcache);
> memcpy (register_buffer (regcache, regnum), buf,
> regcache->descr->sizeof_register[regnum]);
> regcache->register_status[regnum] = REG_VALID;
> +
> + /* Register a cleanup function for invalidating the register after it is
> + written, in case of a failure. */
> + chain_before_invalidate_register =
> + make_cleanup_regcache_invalidate (regcache, regnum);
> +
> target_store_registers (regcache, regnum);
>
> - do_cleanups (old_chain);
> + /* The target did not throw an error so we can discard invalidating the
> + register and restore the cleanup chain to what it was. */
> + discard_cleanups (chain_before_invalidate_register);
> +
> + do_cleanups (chain_before_save_inferior);
> }
>
> void
> diff --git a/gdb/regcache.h b/gdb/regcache.h
> index 8423f57..bb40b65 100644
> --- a/gdb/regcache.h
> +++ b/gdb/regcache.h
> @@ -33,6 +33,8 @@ extern struct regcache *get_thread_arch_aspace_regcache (ptid_t,
>
> void regcache_xfree (struct regcache *regcache);
> struct cleanup *make_cleanup_regcache_xfree (struct regcache *regcache);
> +struct cleanup *make_cleanup_regcache_invalidate (struct regcache *regcache,
> + int regnum);
> struct regcache *regcache_xmalloc (struct gdbarch *gdbarch,
> struct address_space *aspace);
>
^ permalink raw reply [flat|nested] 6+ messages in thread* [PING^2][RFC][PATCH v2][PR remote/16896] Invalidate a register in cache when a remote target failed to write it.
2014-06-02 9:12 ` [PING][RFC][PATCH " Pierre Langlois
@ 2014-06-11 16:58 ` Pierre Langlois
0 siblings, 0 replies; 6+ messages in thread
From: Pierre Langlois @ 2014-06-11 16:58 UTC (permalink / raw)
To: gdb-patches, palves
Ping.
On 02/06/14 10:12, Pierre Langlois wrote:
> Ping.
>
> On 22/05/14 15:23, Pierre Langlois wrote:
>> Hello all,
>>
>> This patch addresses the issue in patch [1] generically as pointed out by
>> a comment [2].
>>
>> As opposed to invalidating a register in cache in the remote target before
>> throwing an error, we can do this using a cleanup in regcache_raw_write. This
>> patch adds routines to add a regcache_invalidate cleanup to the current chain.
>> We can use this before target_store_registers and discard it after if an error
>> was not thrown.
>>
>> [1] https://sourceware.org/ml/gdb-patches/2014-05/msg00083.html
>> [2] https://sourceware.org/ml/gdb-patches/2014-05/msg00357.html
>>
>> 2014-05-20 Pierre Langlois <pierre.langlois@embecosm.com>
>>
>> PR remote/16896
>> * regcache.c (register_to_invalidate): New structure. Combines a pointer
>> to a struct regcache and a register number.
>> (do_register_invalidate): New function. Call regcache_invalidate.
>> (make_cleanup_regcache_invalidate): New function. Construct a cleanup
>> for invalidating a register.
>> (regcache_raw_write): Call make_cleanup_regcache_invalidate
>> * regcache.h (make_cleanup_regcache_invalidate): New prototype.
>>
>> ---
>> gdb/regcache.c | 41 ++++++++++++++++++++++++++++++++++++++---
>> gdb/regcache.h | 2 ++
>> 2 files changed, 40 insertions(+), 3 deletions(-)
>>
>> diff --git a/gdb/regcache.c b/gdb/regcache.c
>> index 8b588c6..87a6b02 100644
>> --- a/gdb/regcache.c
>> +++ b/gdb/regcache.c
>> @@ -267,6 +267,30 @@ make_cleanup_regcache_xfree (struct regcache *regcache)
>> return make_cleanup (do_regcache_xfree, regcache);
>> }
>>
>> +/* Cleanup routines for invalidating a register. */
>> +
>> +struct register_to_invalidate
>> +{
>> + struct regcache *regcache;
>> + int regnum;
>> +};
>> +
>> +static void
>> +do_regcache_invalidate (void *data)
>> +{
>> + struct register_to_invalidate *reg = data;
>> + regcache_invalidate (reg->regcache, reg->regnum);
>> +}
>> +
>> +struct cleanup *
>> +make_cleanup_regcache_invalidate (struct regcache *regcache, int regnum)
>> +{
>> + struct register_to_invalidate* reg = XNEW (struct register_to_invalidate);
>> + reg->regcache = regcache;
>> + reg->regnum = regnum;
>> + return make_cleanup_dtor (do_regcache_invalidate, (void *) reg, xfree);
>> +}
>> +
>> /* Return REGCACHE's architecture. */
>>
>> struct gdbarch *
>> @@ -846,7 +870,8 @@ void
>> regcache_raw_write (struct regcache *regcache, int regnum,
>> const gdb_byte *buf)
>> {
>> - struct cleanup *old_chain;
>> + struct cleanup *chain_before_save_inferior;
>> + struct cleanup *chain_before_invalidate_register;
>>
>> gdb_assert (regcache != NULL && buf != NULL);
>> gdb_assert (regnum >= 0 && regnum < regcache->descr->nr_raw_registers);
>> @@ -864,16 +889,26 @@ regcache_raw_write (struct regcache *regcache, int regnum,
>> regcache->descr->sizeof_register[regnum]) == 0))
>> return;
>>
>> - old_chain = save_inferior_ptid ();
>> + chain_before_save_inferior = save_inferior_ptid ();
>> inferior_ptid = regcache->ptid;
>>
>> target_prepare_to_store (regcache);
>> memcpy (register_buffer (regcache, regnum), buf,
>> regcache->descr->sizeof_register[regnum]);
>> regcache->register_status[regnum] = REG_VALID;
>> +
>> + /* Register a cleanup function for invalidating the register after it is
>> + written, in case of a failure. */
>> + chain_before_invalidate_register =
>> + make_cleanup_regcache_invalidate (regcache, regnum);
>> +
>> target_store_registers (regcache, regnum);
>>
>> - do_cleanups (old_chain);
>> + /* The target did not throw an error so we can discard invalidating the
>> + register and restore the cleanup chain to what it was. */
>> + discard_cleanups (chain_before_invalidate_register);
>> +
>> + do_cleanups (chain_before_save_inferior);
>> }
>>
>> void
>> diff --git a/gdb/regcache.h b/gdb/regcache.h
>> index 8423f57..bb40b65 100644
>> --- a/gdb/regcache.h
>> +++ b/gdb/regcache.h
>> @@ -33,6 +33,8 @@ extern struct regcache *get_thread_arch_aspace_regcache (ptid_t,
>>
>> void regcache_xfree (struct regcache *regcache);
>> struct cleanup *make_cleanup_regcache_xfree (struct regcache *regcache);
>> +struct cleanup *make_cleanup_regcache_invalidate (struct regcache *regcache,
>> + int regnum);
>> struct regcache *regcache_xmalloc (struct gdbarch *gdbarch,
>> struct address_space *aspace);
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC][PATCH v2][PR remote/16896] Invalidate a register in cache when a remote target failed to write it.
2014-05-22 14:23 [RFC][PATCH v2][PR remote/16896] Invalidate a register in cache when a remote target failed to write it Pierre Langlois
2014-06-02 9:12 ` [PING][RFC][PATCH " Pierre Langlois
@ 2014-06-12 13:23 ` Pedro Alves
2014-06-12 16:03 ` Pierre Langlois
1 sibling, 1 reply; 6+ messages in thread
From: Pedro Alves @ 2014-06-12 13:23 UTC (permalink / raw)
To: Pierre Langlois, gdb-patches
Hi Pierre,
On 05/22/2014 03:23 PM, Pierre Langlois wrote:
> Hello all,
>
> This patch addresses the issue in patch [1] generically as pointed out by
> a comment [2].
>
> As opposed to invalidating a register in cache in the remote target before
> throwing an error, we can do this using a cleanup in regcache_raw_write. This
> patch adds routines to add a regcache_invalidate cleanup to the current chain.
> We can use this before target_store_registers and discard it after if an error
> was not thrown.
>
> [1] https://sourceware.org/ml/gdb-patches/2014-05/msg00083.html
> [2] https://sourceware.org/ml/gdb-patches/2014-05/msg00357.html
Based on the commit log / description of the change in v1, could
you propose a commit log for this version of the patch, please?
> 2014-05-20 Pierre Langlois <pierre.langlois@embecosm.com>
>
> PR remote/16896
> * regcache.c (register_to_invalidate): New structure. Combines a pointer
Double space after periods. "structure. Combines".
But just "New structure" is enough. The structure's description should be
in the code. We usually spell out "struct" in the context part as well. So:
* regcache.c (struct register_to_invalidate): New structure.
> to a struct regcache and a register number.
> (do_register_invalidate): New function. Call regcache_invalidate.
> (make_cleanup_regcache_invalidate): New function. Construct a cleanup
> for invalidating a register.
> (regcache_raw_write): Call make_cleanup_regcache_invalidate
Missing period.
Also, please make sure ChangeLog entries are indented with a tab instead
of spaces.
I suggest:
* regcache.c (struct register_to_invalidate): New structure.
(do_register_invalidate, make_cleanup_regcache_invalidate): New
functions.
(regcache_raw_write): Call make_cleanup_regcache_invalidate.
* regcache.h (make_cleanup_regcache_invalidate): New prototype.
> +
> +static void
> +do_regcache_invalidate (void *data)
> +{
> + struct register_to_invalidate *reg = data;
> + regcache_invalidate (reg->regcache, reg->regnum);
Empty line after declaration.
> +}
> +
> +struct cleanup *
> +make_cleanup_regcache_invalidate (struct regcache *regcache, int regnum)
As nothing is calling this outside regcache.c, please make it static.
> +{
> + struct register_to_invalidate* reg = XNEW (struct register_to_invalidate);
> + reg->regcache = regcache;
Empty line after declaration.
Thanks!
--
Pedro Alves
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC][PATCH v2][PR remote/16896] Invalidate a register in cache when a remote target failed to write it.
2014-06-12 13:23 ` [RFC][PATCH " Pedro Alves
@ 2014-06-12 16:03 ` Pierre Langlois
2014-06-12 16:31 ` Pedro Alves
0 siblings, 1 reply; 6+ messages in thread
From: Pierre Langlois @ 2014-06-12 16:03 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
Hi Pedro,
>> +}
>> +
>> +struct cleanup *
>> +make_cleanup_regcache_invalidate (struct regcache *regcache, int regnum)
>
> As nothing is calling this outside regcache.c, please make it static.
>
Should I exclude make_cleanup_regcache_invalidate from regcache.h as well in
this case?
Thank you for the review! And sorry for the formatting errors.
Best,
Pierre
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-06-12 16:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-22 14:23 [RFC][PATCH v2][PR remote/16896] Invalidate a register in cache when a remote target failed to write it Pierre Langlois
2014-06-02 9:12 ` [PING][RFC][PATCH " Pierre Langlois
2014-06-11 16:58 ` [PING^2][RFC][PATCH " Pierre Langlois
2014-06-12 13:23 ` [RFC][PATCH " Pedro Alves
2014-06-12 16:03 ` Pierre Langlois
2014-06-12 16:31 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox