Re: [PATCH 3/4] utrace_set_events: fix UTRACE_EVENT(REAP) case

[Roland McGrath]

 On 08/16, Roland McGrath wrote:
 
 - It is possible that both -death and -reap are true. In this
   case it is OK to clear UTRACE_EVENT(REAP), but set_events fails.
 
  No, it's not OK to clear it.  Once -reap is set, then the engine's
  ops-report_reap might or might not have been called already.
 
 Afaics - no.
 
 If utrace-death is set (and we check it under utrace-lock) we can
 ignore utrace-reap.
 
 In short, if ops-report_reap can be called before -death is cleared,
 then 2 possible callers of utrace_maybe_reap() can race with each other,
 but this can't happen.

Right.  If -death is still set, it means utrace_report_death is still
running.  So the utrace internals know that the report_reap callbacks
haven't started yet.  But from the utrace API perspective, all you can
know is that release_task() has already been called.  So I think it's
right for the API not to let you clear UTRACE_EVENT(REAP) at that point.

 utrace-death == T means:
 
   - (utrace_flags  _UTRACE_DEATH_EVENTS) == T

Yes.

   - utrace-death was set by utrace_report_death() which will take
 utrace-lock later and clear -death, only then it may call
 ops-report_reap().

Yes.

   - until utrace_report_death() clears -death, _UTRACE_DEATH_EVENTS
 must be set in -utrace_flags, otherwise utrace_maybe_reap(true)
 is buggy.

Yes.

 Note that both utrace-death and _UTRACE_DEATH_EVENTS are cleared
 atomically from utrace-lock pov.

Yes.

 IOW. utrace-death is true, then
 
   IF (utrace-reap)
 
   tracehook_prepare_releas()-utrace_maybe_reap(true) was
   already called, this is how utrace-reap was set.

Meaning release_task() was called--semantically reaping may have begun.

   But utrace_maybe_reap() did nothing and returned.
   We rely on the subsequent utrace_maybe_reap(false) from
   utrace_report_death() - but this can't happen until
   we drop utrace-lock

Correct.

   ELSE
   utrace-reap can't be set until we drop utrace-lock.   

Correct.

 Now that you merged c93fecc925ea7567168f0c94414b9021de2708c5
 get_utrace_lock() must not succeed if utrace-reap == T, this becomes
 a bit off-topic. However, I thought about relaxing the dead check in
 get_utrace_lock(), instead of utrace-reap we could check
 utrace-reap  !utrace-death. In fact, initially I was going to
 do this, but then decided to make the simpler patch for now.

When utrace-reap is set, it means release_task() has been called.  The
API caller has no way to know reaping hasn't already begun--except if
its report_death callback has not returned yet.  But I think that the
API definition of once release_task() has been called (i.e. entered,
maybe not returned yet), any utrace call will get -ESRCH, is a clear
and comprehensible constraint.  We should not open any holes in that.


Thanks,
Roland

Re: [PATCH] fix mark_engine_detached() vs start_callback() race

[Roland McGrath]

 This is the minimal temporary ugly fix for now, we should certainly
 cleanup and simplify this logic. The barriers in mark_engine_detached()
 and in start_callback() can't help and should be removed. If we ignore
 utrace_get_signal() we do not even need utrace_detached_quiesce(),
 start_callback() could just do

Agreed.  I applied the patch for now.

 I think in the longer term mark_engine_detached() should not change
 engine-flags at all but add QUIESCE to -utrace_flags. However, this
 breaks utrace_maybe_reap(reap = true) and we should avoid the race
 with finish_callback() which clears -reporting after report_quiesce().

What's the benefit to adding QUIESCE?  If any utrace code gets entered at
all, then any callback run will be able to do the special-case ops check
for detached engines.

 A bit off-topic, but I don't think finish_callback() should check
 engine-ops == utrace_detached_ops before return. Instead we should
 change finish_callback_report() to return the boolean. We shouldn't
 return true without setting report-detaches.

Ok.


Thanks,
Roland

Re: [PATCH?] avoid the unnecessary utrace_resume()-utrace_reset()

[Roland McGrath]

 utrace_resume(UTRACE_REPORT) always calls utrace_reset() because
 start_callback() obviously can't clear report-spurious when
 event == 0.
 
 Change start_callback() to correctly clear -spurious in this case.

Ok.

 Note: utrace_control(DETACH) does utrace_do_stop() and sets UTRACE_REPORT
 if the tracee is not stopped. It also does mark_engine_detached() which
 does not set QUIESCE in target-utrace_flags. This means we rely on
 report.spurious which should provoke utrace_reset() from utrace_resume()
 if target-utrace_flags doesn't have QUIESCE. A bit too subtle, imho.

Agreed.  There is no reason utrace_control can't set it in utrace_flags
in its !reset case.

 Also, UTRACE_REPORT can be lost because of UTRACE_INTERRUPT or normal
 signal: utrace_get_signal() checks utrace_flags  UTRACE_EVENT(QUIESCE)
 and returns otherwise. This should be fixed somehow. This check is wrong
 anyway, but it is not clear how we can fix the race with DETACH.

I see.  That would be fixed by utrace_control setting it.


Thanks,
Roland

Re: gdbstub initial code, v4

[Roland McGrath]

 Note the second attachment, GDBCAT. It is just the simple perl
 script which connects /proc/ugdb to tcp port. It can be used for
 remote debugging via tcp, or with (unpatched) gdb which can't do
 select() on /proc/ugdb. 

bash$ nc -l 2000  /proc/ugdb 

 Actually, it is more convenient to use
 it in any case, at least for logging purposes.

(gdb) set remote debug 1


Thanks,
Roland