Re: [PATCH 4/24] gdb: replace some so_list parameters to use references

[Simon Marchi <simon.marchi@polymtl.ca>]

On 10/19/23 07:07, Lancelot SIX wrote:
> Hi Simon,
> 
> Some minor remarks below.
> 
> On Tue, Oct 10, 2023 at 04:39:59PM -0400, Simon Marchi wrote:
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>>
>> A subsequent patch changes so_list to be linked using
>> intrusive_list.  Iterating an intrusive_list yields some references to
>> the list elements.  Convert some functions accepting so_list objects to
>> take references, to make things easier and more natural.  Add const
>> where possible and convenient.
>>
>> Change-Id: Id5ab5339c3eb6432e809ad14782952d6a45806f3
>> ---
>>  gdb/breakpoint.c     |   5 +-
>>  gdb/bsd-uthread.c    |  12 ++---
>>  gdb/exec.c           |   4 +-
>>  gdb/interps.c        |   4 +-
>>  gdb/interps.h        |   8 +--
>>  gdb/mi/mi-cmd-file.c |   2 +-
>>  gdb/mi/mi-interp.c   |  26 ++++-----
>>  gdb/mi/mi-interp.h   |   6 +--
>>  gdb/nto-tdep.c       |   6 +--
>>  gdb/nto-tdep.h       |   3 +-
>>  gdb/observable.h     |   5 +-
>>  gdb/progspace.h      |   4 +-
>>  gdb/solib-aix.c      |  11 ++--
>>  gdb/solib-darwin.c   |  23 ++++----
>>  gdb/solib-dsbt.c     |   9 ++--
>>  gdb/solib-frv.c      |   9 ++--
>>  gdb/solib-rocm.c     |   8 +--
>>  gdb/solib-svr4.c     |  35 ++++++------
>>  gdb/solib-target.c   |  49 +++++++++--------
>>  gdb/solib.c          | 126 +++++++++++++++++++++----------------------
>>  gdb/solib.h          |   4 +-
>>  gdb/solist.h         |  13 +++--
>>  gdb/target-section.h |   2 +-
>>  23 files changed, 183 insertions(+), 191 deletions(-)
>>
>> diff --git a/gdb/mi/mi-interp.h b/gdb/mi/mi-interp.h
>> index f9af61f0a571..781a8dc6f466 100644
>> --- a/gdb/mi/mi-interp.h
>> +++ b/gdb/mi/mi-interp.h
>> @@ -60,8 +60,8 @@ class mi_interp final : public interp
>>    void on_record_changed (inferior *inf, int started, const char *method,
>>  			  const char *format) override;
>>    void on_target_resumed (ptid_t ptid) override;
>> -  void on_solib_loaded (so_list *so) override;
>> -  void on_solib_unloaded (so_list *so) override;
>> +  void on_solib_loaded (const so_list &so) override;
>> +  void on_solib_unloaded (const so_list &so) override;
> 
> It is orthogonal to this change, but it would make sense for those
> methods to be const as well.
> 
> Doing this requires interp::interp_ui_out to be const as well (as done
> in attached patch).

Why do you think the interp object should be const (which is the effect
of marking the methods const)?  These methods are notifiers to let the
interp know about certain events that occured.  Whether they should
modify the state of the interpreter or not is up to the particular
implementation of the interpreter.  Perhaps you're right, but I don't
see it.

In your patch, you make interp::interp_ui_out const, but it still
returns a non-const ui_out.  So when outputting something, the
interpreter itself is not modified, but the ui_out behind it is.  I
presume (sometimes) the ui_out needs to be non-const, since for things
like pagination it clearly needs to keep a state.  Design-wise, is it
"correct" to make a const intepreter able to return a non-const ui_out?

>>    void on_about_to_proceed () override;
>>    void on_traceframe_changed (int tfnum, int tpnum) override;
>>    void on_tsv_created (const trace_state_variable *tsv) override;
>> diff --git a/gdb/observable.h b/gdb/observable.h
>> index acb05e9b535c..5ed6ca547ce0 100644
>> --- a/gdb/observable.h
>> +++ b/gdb/observable.h
>> @@ -99,13 +99,12 @@ extern observable<inferior */* parent_inf */, inferior */* child_inf */,
>>  /* The shared library specified by SOLIB has been loaded.  Note that
>>     when gdb calls this observer, the library's symbols probably
>>     haven't been loaded yet.  */
>> -extern observable<struct so_list */* solib */> solib_loaded;
>> +extern observable<so_list &/* solib */> solib_loaded;
> 
> I am wondering, is there a reason to make the solib parameter const for
> solib_unloaded but not for solib_loaded?  
> 
> Changing it to const seems to still compile just fine.  If down the line
> an observer needs to modify the SO, the observer's signature can be
> adjusted.

Just pragmatic reasons.  Do you build with --enable-targets=all?

It's because bsd_uthread_solib_loaded calls solib_read_symbols to read
in the symbols of the library it is looking for, and that requires a
non-const so_list.

Simon