catch load/unload not implemented on any target (remove?)

catch load/unload not implemented on any target (remove?)

[Joel Brobecker]

Hello,

I was looking at converting the catch load/unload implementation
to using the bp_catchpoint kind. But looking at the implementation,
I realized there isn't any platform where this feature is implemented.
The documentation says HP/UX, but this isn't correct either, AFAICT.

The "protocol" between the breakpoint solib layers is based on
some macros being defined:

    SOLIB_LOADED_LIBRARY_PATHNAME(pid)
    LIB_UNLOADED_LIBRARY_PATHNAME(pid)
    SOLIB_CREATE_CATCH_LOAD_HOOK(pid,tempflag,filename,cond_string)
    SOLIB_CREATE_CATCH_UNLOAD_HOOK(pid, tempflag, filename, cond_string)

These macros used to be defined in the various target-dependent solib
files. For instance, in gdb-5.3, it was defined in: coff-solib.h,
pa64solib.h, solib.h, and somsolib.h.

I think that the mechanism of using macros is definitely OBE now, and
one should use "methods" in the target_so_ops structure.

I am conflicted as to what to do in the meantime: Leave the code as is,
and update the documentation that this feature is currently implemented
on no platform. Or remove the code entirely.

I am leaning towards removing the code entirely, for several reasons:
  - Documentating a feature as unimplemented seems silly;
  - I don't think there is much in the current code that once can
    reuse in order to implement this feature properly
  - When someone is ready to implement this feature again for his platform,
    there shouldn't be much to do in terms of infrastructure work to do at
    the breakpoint module level.  So it shouldn't be very difficult to
    implement. There might be one little difficulty based on the fact
    that some architectures will implement this feature using a phyical
    breakpoint (eg: svr4) whereas others won't (eg: Windows), but that
    shouldn't be very difficult to handle by using the right bp_kind.

Thoughts?

-- 
Joel

Re: catch load/unload not implemented on any target (remove?)

[Eli Zaretskii]

> Date: Sun, 26 Oct 2008 17:40:44 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> 
> I am conflicted as to what to do in the meantime: Leave the code as is,
> and update the documentation that this feature is currently implemented
> on no platform. Or remove the code entirely.
> 
> I am leaning towards removing the code entirely, for several reasons:
>   - Documentating a feature as unimplemented seems silly;
>   - I don't think there is much in the current code that once can
>     reuse in order to implement this feature properly
>   - When someone is ready to implement this feature again for his platform,
>     there shouldn't be much to do in terms of infrastructure work to do at
>     the breakpoint module level.  So it shouldn't be very difficult to
>     implement. There might be one little difficulty based on the fact
>     that some architectures will implement this feature using a phyical
>     breakpoint (eg: svr4) whereas others won't (eg: Windows), but that
>     shouldn't be very difficult to handle by using the right bp_kind.
> 
> Thoughts?

Given what you describe, I'm okay with removing that code.

Re: catch load/unload not implemented on any target (remove?)

[Aleksandar Ristovski]

Joel Brobecker wrote:
> Hello,
> 
> I was looking at converting the catch load/unload implementation
> to using the bp_catchpoint kind. But looking at the implementation,
> I realized there isn't any platform where this feature is implemented.
> The documentation says HP/UX, but this isn't correct either, AFAICT.
> 
> The "protocol" between the breakpoint solib layers is based on
> some macros being defined:
> 
>     SOLIB_LOADED_LIBRARY_PATHNAME(pid)
>     LIB_UNLOADED_LIBRARY_PATHNAME(pid)
>     SOLIB_CREATE_CATCH_LOAD_HOOK(pid,tempflag,filename,cond_string)
>     SOLIB_CREATE_CATCH_UNLOAD_HOOK(pid, tempflag, filename, cond_string)
> 
> These macros used to be defined in the various target-dependent solib
> files. For instance, in gdb-5.3, it was defined in: coff-solib.h,
> pa64solib.h, solib.h, and somsolib.h.
> 
> I think that the mechanism of using macros is definitely OBE now, and
> one should use "methods" in the target_so_ops structure.

I agree.

> 
> I am conflicted as to what to do in the meantime: Leave the code as is,
> and update the documentation that this feature is currently implemented
> on no platform. Or remove the code entirely.
> 
> I am leaning towards removing the code entirely, for several reasons:
>   - Documentating a feature as unimplemented seems silly;

I agree.

>   - I don't think there is much in the current code that once can
>     reuse in order to implement this feature properly
>   - When someone is ready to implement this feature again for his platform,
>     there shouldn't be much to do in terms of infrastructure work to do at
>     the breakpoint module level.  So it shouldn't be very difficult to
>     implement. There might be one little difficulty based on the fact
>     that some architectures will implement this feature using a phyical
>     breakpoint (eg: svr4) whereas others won't (eg: Windows), but that
>     shouldn't be very difficult to handle by using the right bp_kind.
> 

Well, I had to implement catch load/unload; their appearance 
in the list of catchpoints set some expectations which 
evolved into requirements.

I used the above mentioned macros.

Thanks,

Aleksandar

Re: catch load/unload not implemented on any target (remove?)

[Aleksandar Ristovski]

Joel Brobecker wrote:
> Hello,
> 
> I was looking at converting the catch load/unload implementation
> to using the bp_catchpoint kind. But looking at the implementation,
> I realized there isn't any platform where this feature is implemented.
> The documentation says HP/UX, but this isn't correct either, AFAICT.
> 
> The "protocol" between the breakpoint solib layers is based on
> some macros being defined:
> 
>     SOLIB_LOADED_LIBRARY_PATHNAME(pid)
>     LIB_UNLOADED_LIBRARY_PATHNAME(pid)
>     SOLIB_CREATE_CATCH_LOAD_HOOK(pid,tempflag,filename,cond_string)
>     SOLIB_CREATE_CATCH_UNLOAD_HOOK(pid, tempflag, filename, cond_string)
> 
> These macros used to be defined in the various target-dependent solib
> files. For instance, in gdb-5.3, it was defined in: coff-solib.h,
> pa64solib.h, solib.h, and somsolib.h.
> 
> I think that the mechanism of using macros is definitely OBE now, and
> one should use "methods" in the target_so_ops structure.

I agree.

> 
> I am conflicted as to what to do in the meantime: Leave the code as is,
> and update the documentation that this feature is currently implemented
> on no platform. Or remove the code entirely.
> 
> I am leaning towards removing the code entirely, for several reasons:
>   - Documentating a feature as unimplemented seems silly;

I agree.

>   - I don't think there is much in the current code that once can
>     reuse in order to implement this feature properly
>   - When someone is ready to implement this feature again for his platform,
>     there shouldn't be much to do in terms of infrastructure work to do at
>     the breakpoint module level.  So it shouldn't be very difficult to
>     implement. There might be one little difficulty based on the fact
>     that some architectures will implement this feature using a phyical
>     breakpoint (eg: svr4) whereas others won't (eg: Windows), but that
>     shouldn't be very difficult to handle by using the right bp_kind.
> 

Well, I had to implement catch load/unload; their appearance 
in the list of catchpoints set some expectations which 
evolved into requirements.

I used the above mentioned macros.

Thanks,

Aleksandar

Re: catch load/unload not implemented on any target (remove?)

[Daniel Jacobowitz]

On Sun, Oct 26, 2008 at 05:40:44PM -0700, Joel Brobecker wrote:
> Hello,
> 
> I was looking at converting the catch load/unload implementation
> to using the bp_catchpoint kind. But looking at the implementation,
> I realized there isn't any platform where this feature is implemented.
> The documentation says HP/UX, but this isn't correct either, AFAICT.

It used to be, IIRC.

> I think that the mechanism of using macros is definitely OBE now, and
> one should use "methods" in the target_so_ops structure.

I think it should be even higher level than that.  The core solib
machinery knows how to stop on load/unload (set stop-on-solib-events).
And it knows how to update the list of loaded libraries based on
current_sos.  All that's missing is a 'diff' operation: report which
libraries have been added or removed, so that the common code can
report them sensibly to the user.  I'd love if I could replace "set
stop-on-solib-events 1" with "catch load" and have GDB say "Stopped at
load of libfoo.so.1" or "Stopped at load of 6 libraries".  The latter
is not as important, since (on svr4 targets) it happens only during
program startup, when the dynamic loader initializes.

IOW I don't think we need any new breakpoint that we don't already
have.

-- 
Daniel Jacobowitz
CodeSourcery

Re: catch load/unload not implemented on any target (remove?)

[Joel Brobecker]

> I think it should be even higher level than that.  The core solib
> machinery knows how to stop on load/unload (set stop-on-solib-events).
> And it knows how to update the list of loaded libraries based on
> current_sos.  All that's missing is a 'diff' operation: report which
> libraries have been added or removed, so that the common code can
> report them sensibly to the user.  I'd love if I could replace "set
> stop-on-solib-events 1" with "catch load" and have GDB say "Stopped at
> load of libfoo.so.1" or "Stopped at load of 6 libraries".  The latter
> is not as important, since (on svr4 targets) it happens only during
> program startup, when the dynamic loader initializes.
> 
> IOW I don't think we need any new breakpoint that we don't already
> have.

I thought about that too. This would work very well with targets
that receive an event (either a breakpoint or a specific notification
like on Windows) immediately when a shared library is loaded or unloaded.
But I wonder if we might have some targets where this is not the case.
It it were the case, the notications would arrive late for these targets.

I looked at all the solib-* files, and the only target where that might
be the case is Tru64 - init_inferior_hook isn't turning on any event or
inserting any shlib breakpoint, and procfs doesn't seem to provide
handling for shlib notifications. Perhaps what we could do is error-out
if the user tries to insert a catchpoint on load/unload events, then.

In any case, looks like there a general agreement to remove the current
code... I will do that after I have submitted the changes I made to
get rid of bp_catch_exec.

-- 
Joel