Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Joel Brobecker <brobecker@adacore.com>
To: Jan Kratochvil <jan.kratochvil@redhat.com>
Cc: gdb-patches@sourceware.org
Subject: Re: [patch 2/4] Fix hw watchpoints: reordered / simultaneously hit 	[fixup #1]
Date: Fri, 02 Oct 2009 23:01:00 -0000	[thread overview]
Message-ID: <20091002230124.GG10338@adacore.com> (raw)
In-Reply-To: <20091002221254.GA7767@host0.dyn.jankratochvil.net>

Jan,

> the patch got extended by this incremental change (fixing a regression):
>     
> gdb/
> 	Fix assertion failure with `set debug infrun 1'.
> 	* infrun.c (handle_inferior_event <debug_infrun>): New variable
> 	old_chain.  Temporarily switch INFERIOR_PTID.
> 	* target.h (target_stopped_by_watchpoint): Extend the comment.
> 	(target_stopped_data_address): New comment.  Rename X as ADDR_P.

Can we treat this part independently from the rest? I assume that
this is a latent issue that only gets uncovered by your watchpoint
patch? I think the bug is there regardless, and even if we can't test
it now, it's going to simplify a bit my task if we treat this piece
independently.

I think that your fix is not optimal.  Here is what I suggest instead:
Make the ptid an explicit parameter in the call to
target_stopped_by_watchpoint. There are two parts to this change,
and I think we can find a way of making the change in two steps:

  - First step: Change the profile of target_stopped_by_watchpoint
    to add a ptid (say as the second argument). We're also trying
    to get rid of these target_... macros, so now is the time to
    turn that macro into a function.  We update all the callers
    to pass the ptid (could be either inferior_ptid or ecs-ptid
    in your infrun case).

    We hold off the update of all the to_stopped_data_address
    callbacks in struct target_ops for now.  This means that we need
    to keep the current interface as is, and that we need to temporarily
    change the inferior_ptid before calling the callback.

  - Second step: Change the callbacks interface, and update all
    the callbacks. This is trickier.  For the platforms that we can test,
    I suggest trying to fix the code accordingly.  For the others, do
    the same as before: Start by changing the inferior_ptid during
    the life of the callback.

    This second step might or might not be worth it, as the code
    on which the callbacks depend might not entirely ready.  But
    at least we pushed the mess to the target code.

Since I'm dumping some cleanup work on you, I can take care of the
second part if you are willing to take care of the first one.

-- 
Joel


  reply	other threads:[~2009-10-02 23:01 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-08-17 19:46 [patch 2/4] Fix hw watchpoints: reordered / simultaneously hit Jan Kratochvil
2009-10-02 22:13 ` [patch 2/4] Fix hw watchpoints: reordered / simultaneously hit [fixup #1] Jan Kratochvil
2009-10-02 23:01   ` Joel Brobecker [this message]
2009-10-03 17:23     ` Jan Kratochvil
2009-10-07 18:39       ` Joel Brobecker
2009-10-12 16:00         ` Jan Kratochvil
2009-10-12 18:58           ` Eli Zaretskii
2009-10-13 16:02           ` Jan Kratochvil
2009-11-03  1:12           ` Joel Brobecker

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20091002230124.GG10338@adacore.com \
    --to=brobecker@adacore.com \
    --cc=gdb-patches@sourceware.org \
    --cc=jan.kratochvil@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox