* [PATCH 0/4] Small Misc psymtabs / objfile cleanups @ 2021-03-22 19:34 Simon Marchi 2021-03-22 19:34 ` [PATCH 1/4] gdb: add intern method to objfile_per_bfd_storage Simon Marchi ` (3 more replies) 0 siblings, 4 replies; 16+ messages in thread From: Simon Marchi @ 2021-03-22 19:34 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi I spotted these while looking at Tom's psymtabs series, nothing major. Simon Marchi (4): gdb: add intern method to objfile_per_bfd_storage gdb: use std::string in partial_symtab::partial_symtab / allocate_symtab gdb: pass objfile_per_bfd_storage instead of objfile to partial_symtab gdb: remove objfile parameter from get_objfile_bfd_data gdb/ctfread.c | 6 +++--- gdb/dbxread.c | 4 ++-- gdb/dwarf2/read.c | 18 ++++++++---------- gdb/dwarf2/read.h | 4 ++-- gdb/mdebugread.c | 6 +++--- gdb/objfiles.c | 8 ++++---- gdb/objfiles.h | 29 ++++++++++++++++++++++++----- gdb/psympriv.h | 23 ++++++++++++----------- gdb/psymtab.c | 21 ++++++++++----------- gdb/symfile.c | 11 +++++------ gdb/xcoffread.c | 4 ++-- 11 files changed, 75 insertions(+), 59 deletions(-) -- 2.30.0 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/4] gdb: add intern method to objfile_per_bfd_storage 2021-03-22 19:34 [PATCH 0/4] Small Misc psymtabs / objfile cleanups Simon Marchi @ 2021-03-22 19:34 ` Simon Marchi 2021-03-23 14:47 ` Christian Biesinger 2021-03-22 19:34 ` [PATCH 2/4] gdb: use std::string in partial_symtab::partial_symtab / allocate_symtab Simon Marchi ` (2 subsequent siblings) 3 siblings, 1 reply; 16+ messages in thread From: Simon Marchi @ 2021-03-22 19:34 UTC (permalink / raw) To: gdb-patches From: Simon Marchi <simon.marchi@polymtl.ca> This allows factoring out the internal implementation details that are currently in the two objfile::intern methods. gdb/ChangeLog: * objfiles.h (struct objfile_per_bfd_storage) <intern>: New method. (struct objfile) <intern>: Use objfile::objfile_per_bfd_storage::intern. Change-Id: Ifd54026c5efaeffafac9b84ff84c199acc7ce78a --- gdb/objfiles.h | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/gdb/objfiles.h b/gdb/objfiles.h index 41f8fc913d8..d9aa06636f5 100644 --- a/gdb/objfiles.h +++ b/gdb/objfiles.h @@ -270,6 +270,14 @@ struct objfile_per_bfd_storage ~objfile_per_bfd_storage (); + /* Intern STRING in this object's string cache and return the unique copy. + The copy has the same lifetime as this object. */ + + const char *intern (gdb::string_view str) + { + return (const char *) string_cache.insert (str.data (), str.size () + 1); + } + /* The storage has an obstack of its own. */ auto_obstack storage_obstack; @@ -516,15 +524,14 @@ struct objfile lifetime as the per-BFD object. */ const char *intern (const char *str) { - return (const char *) per_bfd->string_cache.insert (str, strlen (str) + 1); + return per_bfd->intern (str); } /* Intern STRING and return the unique copy. The copy has the same lifetime as the per-BFD object. */ const char *intern (const std::string &str) { - return (const char *) per_bfd->string_cache.insert (str.c_str (), - str.size () + 1); + return per_bfd->intern (str); } /* Retrieve the gdbarch associated with this objfile. */ -- 2.30.0 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] gdb: add intern method to objfile_per_bfd_storage 2021-03-22 19:34 ` [PATCH 1/4] gdb: add intern method to objfile_per_bfd_storage Simon Marchi @ 2021-03-23 14:47 ` Christian Biesinger 2021-03-23 15:48 ` Simon Marchi 0 siblings, 1 reply; 16+ messages in thread From: Christian Biesinger @ 2021-03-23 14:47 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches On Mon, Mar 22, 2021 at 8:34 PM Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> wrote: > > From: Simon Marchi <simon.marchi@polymtl.ca> > > This allows factoring out the internal implementation details that are > currently in the two objfile::intern methods. > > gdb/ChangeLog: > > * objfiles.h (struct objfile_per_bfd_storage) <intern>: New > method. > (struct objfile) <intern>: Use > objfile::objfile_per_bfd_storage::intern. > > Change-Id: Ifd54026c5efaeffafac9b84ff84c199acc7ce78a > --- > gdb/objfiles.h | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/gdb/objfiles.h b/gdb/objfiles.h > index 41f8fc913d8..d9aa06636f5 100644 > --- a/gdb/objfiles.h > +++ b/gdb/objfiles.h > @@ -270,6 +270,14 @@ struct objfile_per_bfd_storage > > ~objfile_per_bfd_storage (); > > + /* Intern STRING in this object's string cache and return the unique copy. > + The copy has the same lifetime as this object. */ > + > + const char *intern (gdb::string_view str) > + { > + return (const char *) string_cache.insert (str.data (), str.size () + 1); Is this guaranteed to be nullterminated even without calling c_str()? Christian ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] gdb: add intern method to objfile_per_bfd_storage 2021-03-23 14:47 ` Christian Biesinger @ 2021-03-23 15:48 ` Simon Marchi 2021-03-23 15:53 ` Christian Biesinger 2021-04-01 17:44 ` Tom Tromey 0 siblings, 2 replies; 16+ messages in thread From: Simon Marchi @ 2021-03-23 15:48 UTC (permalink / raw) To: Christian Biesinger, Simon Marchi; +Cc: gdb-patches On 2021-03-23 10:47 a.m., Christian Biesinger via Gdb-patches wrote: > On Mon, Mar 22, 2021 at 8:34 PM Simon Marchi via Gdb-patches > <gdb-patches@sourceware.org> wrote: >> >> From: Simon Marchi <simon.marchi@polymtl.ca> >> >> This allows factoring out the internal implementation details that are >> currently in the two objfile::intern methods. >> >> gdb/ChangeLog: >> >> * objfiles.h (struct objfile_per_bfd_storage) <intern>: New >> method. >> (struct objfile) <intern>: Use >> objfile::objfile_per_bfd_storage::intern. >> >> Change-Id: Ifd54026c5efaeffafac9b84ff84c199acc7ce78a >> --- >> gdb/objfiles.h | 13 ++++++++++--- >> 1 file changed, 10 insertions(+), 3 deletions(-) >> >> diff --git a/gdb/objfiles.h b/gdb/objfiles.h >> index 41f8fc913d8..d9aa06636f5 100644 >> --- a/gdb/objfiles.h >> +++ b/gdb/objfiles.h >> @@ -270,6 +270,14 @@ struct objfile_per_bfd_storage >> >> ~objfile_per_bfd_storage (); >> >> + /* Intern STRING in this object's string cache and return the unique copy. >> + The copy has the same lifetime as this object. */ >> + >> + const char *intern (gdb::string_view str) >> + { >> + return (const char *) string_cache.insert (str.data (), str.size () + 1); > > Is this guaranteed to be nullterminated even without calling c_str()? > > Christian From what I remember from last time something like this came up (I can't remember the context) is that we'd only use string_view for strings that are null-terminated, even though string_view doesn't guarantee it. Is that the case? Or is it a bad use case for string_view? In this particular case, callers of this intern method have a C string and an std::string, so we know they are null-terminated. Simon ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] gdb: add intern method to objfile_per_bfd_storage 2021-03-23 15:48 ` Simon Marchi @ 2021-03-23 15:53 ` Christian Biesinger 2021-03-23 16:08 ` Simon Marchi 2021-04-01 17:44 ` Tom Tromey 1 sibling, 1 reply; 16+ messages in thread From: Christian Biesinger @ 2021-03-23 15:53 UTC (permalink / raw) To: Simon Marchi; +Cc: Simon Marchi, gdb-patches On Tue, Mar 23, 2021 at 4:49 PM Simon Marchi <simon.marchi@polymtl.ca> wrote: > > On 2021-03-23 10:47 a.m., Christian Biesinger via Gdb-patches wrote: > > On Mon, Mar 22, 2021 at 8:34 PM Simon Marchi via Gdb-patches > > <gdb-patches@sourceware.org> wrote: > >> > >> From: Simon Marchi <simon.marchi@polymtl.ca> > >> > >> This allows factoring out the internal implementation details that are > >> currently in the two objfile::intern methods. > >> > >> gdb/ChangeLog: > >> > >> * objfiles.h (struct objfile_per_bfd_storage) <intern>: New > >> method. > >> (struct objfile) <intern>: Use > >> objfile::objfile_per_bfd_storage::intern. > >> > >> Change-Id: Ifd54026c5efaeffafac9b84ff84c199acc7ce78a > >> --- > >> gdb/objfiles.h | 13 ++++++++++--- > >> 1 file changed, 10 insertions(+), 3 deletions(-) > >> > >> diff --git a/gdb/objfiles.h b/gdb/objfiles.h > >> index 41f8fc913d8..d9aa06636f5 100644 > >> --- a/gdb/objfiles.h > >> +++ b/gdb/objfiles.h > >> @@ -270,6 +270,14 @@ struct objfile_per_bfd_storage > >> > >> ~objfile_per_bfd_storage (); > >> > >> + /* Intern STRING in this object's string cache and return the unique copy. > >> + The copy has the same lifetime as this object. */ > >> + > >> + const char *intern (gdb::string_view str) > >> + { > >> + return (const char *) string_cache.insert (str.data (), str.size () + 1); > > > > Is this guaranteed to be nullterminated even without calling c_str()? > > > > Christian > > From what I remember from last time something like this came up (I can't > remember the context) is that we'd only use string_view for strings that > are null-terminated, even though string_view doesn't guarantee it. Is > that the case? Or is it a bad use case for string_view? Hm, I'm not sure we concluded that in the general case. It came up specifically for general_symbol_info::compute_and_set_names, which can handle the case where it may or may not be nullterminated depending on a parameter. IMO you should at least comment that the string view must be nullterminated here. > In this particular case, callers of this intern method have a C string > and an std::string, so we know they are null-terminated. OK, makes sense. Christian ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] gdb: add intern method to objfile_per_bfd_storage 2021-03-23 15:53 ` Christian Biesinger @ 2021-03-23 16:08 ` Simon Marchi 0 siblings, 0 replies; 16+ messages in thread From: Simon Marchi @ 2021-03-23 16:08 UTC (permalink / raw) To: Christian Biesinger; +Cc: Simon Marchi, gdb-patches >> From what I remember from last time something like this came up (I can't >> remember the context) is that we'd only use string_view for strings that >> are null-terminated, even though string_view doesn't guarantee it. Is >> that the case? Or is it a bad use case for string_view? > > Hm, I'm not sure we concluded that in the general case. It came up > specifically for general_symbol_info::compute_and_set_names, which can > handle the case where it may or may not be nullterminated depending on > a parameter. IMO you should at least comment that the string view must > be nullterminated here. Good idea, will do. Simon ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] gdb: add intern method to objfile_per_bfd_storage 2021-03-23 15:48 ` Simon Marchi 2021-03-23 15:53 ` Christian Biesinger @ 2021-04-01 17:44 ` Tom Tromey 2021-04-02 15:38 ` Simon Marchi 1 sibling, 1 reply; 16+ messages in thread From: Tom Tromey @ 2021-04-01 17:44 UTC (permalink / raw) To: Simon Marchi via Gdb-patches Cc: Christian Biesinger, Simon Marchi, Simon Marchi >>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: Simon> In this particular case, callers of this intern method have a C string Simon> and an std::string, so we know they are null-terminated. It seems a little dangerous to do this, though, in that someone might add a call without remembering or checking the invariant. Maybe it's better to just have 2 overloads on the per-bfd object. Tom ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] gdb: add intern method to objfile_per_bfd_storage 2021-04-01 17:44 ` Tom Tromey @ 2021-04-02 15:38 ` Simon Marchi 0 siblings, 0 replies; 16+ messages in thread From: Simon Marchi @ 2021-04-02 15:38 UTC (permalink / raw) To: Tom Tromey, Simon Marchi via Gdb-patches On 2021-04-01 1:44 p.m., Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: > > Simon> In this particular case, callers of this intern method have a C string > Simon> and an std::string, so we know they are null-terminated. > > It seems a little dangerous to do this, though, in that someone might > add a call without remembering or checking the invariant. > Maybe it's better to just have 2 overloads on the per-bfd object. Yeah, you're right. I changed the patch to just move the two overloads from objfile to objfile_per_bfd_storage. Here's what I pushed: From 4a4f97c129b26445ff14d0e5323feeb80610a539 Mon Sep 17 00:00:00 2001 From: Simon Marchi <simon.marchi@polymtl.ca> Date: Fri, 2 Apr 2021 11:23:52 -0400 Subject: [PATCH] gdb: add intern methods to objfile_per_bfd_storage This allows keeping the objfile_per_bfd_storage implementation details into objfile_per_bfd_storage, instead of into objfile. And this makes the intern methods usable for code that only has an objfile_per_bfd_storage to work with. gdb/ChangeLog: * objfiles.h (struct objfile_per_bfd_storage) <intern>: New methods. (struct objfile) <intern>: Use objfile::objfile_per_bfd_storage::intern. Change-Id: Ifd54026c5efaeffafac9b84ff84c199acc7ce78a --- gdb/ChangeLog | 7 +++++++ gdb/objfiles.h | 22 +++++++++++++++++++--- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 1bdd652b632a..ac64f5c76099 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,10 @@ +2021-04-02 Simon Marchi <simon.marchi@polymtl.ca> + + * objfiles.h (struct objfile_per_bfd_storage) <intern>: New + methods. + (struct objfile) <intern>: Use + objfile::objfile_per_bfd_storage::intern. + 2021-04-01 Simon Marchi <simon.marchi@efficios.com> * gdbtypes.h (TYPE_FLAG_ENUM): Remove, replace all uses diff --git a/gdb/objfiles.h b/gdb/objfiles.h index 4cd392bd7f02..e8a8b5f6de78 100644 --- a/gdb/objfiles.h +++ b/gdb/objfiles.h @@ -270,6 +270,23 @@ struct objfile_per_bfd_storage ~objfile_per_bfd_storage (); + /* Intern STRING in this object's string cache and return the unique copy. + The copy has the same lifetime as this object. + + STRING must be null-terminated. */ + + const char *intern (const char *str) + { + return (const char *) string_cache.insert (str, strlen (str) + 1); + } + + /* Same as the above, but for an std::string. */ + + const char *intern (const std::string &str) + { + return (const char *) string_cache.insert (str.c_str (), str.size () + 1); + } + /* The storage has an obstack of its own. */ auto_obstack storage_obstack; @@ -516,15 +533,14 @@ struct objfile lifetime as the per-BFD object. */ const char *intern (const char *str) { - return (const char *) per_bfd->string_cache.insert (str, strlen (str) + 1); + return per_bfd->intern (str); } /* Intern STRING and return the unique copy. The copy has the same lifetime as the per-BFD object. */ const char *intern (const std::string &str) { - return (const char *) per_bfd->string_cache.insert (str.c_str (), - str.size () + 1); + return per_bfd->intern (str); } /* Retrieve the gdbarch associated with this objfile. */ -- 2.30.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/4] gdb: use std::string in partial_symtab::partial_symtab / allocate_symtab 2021-03-22 19:34 [PATCH 0/4] Small Misc psymtabs / objfile cleanups Simon Marchi 2021-03-22 19:34 ` [PATCH 1/4] gdb: add intern method to objfile_per_bfd_storage Simon Marchi @ 2021-03-22 19:34 ` Simon Marchi 2021-04-01 17:43 ` Tom Tromey 2021-03-22 19:34 ` [PATCH 3/4] gdb: pass objfile_per_bfd_storage instead of objfile to partial_symtab Simon Marchi 2021-03-22 19:34 ` [PATCH 4/4] gdb: remove objfile parameter from get_objfile_bfd_data Simon Marchi 3 siblings, 1 reply; 16+ messages in thread From: Simon Marchi @ 2021-03-22 19:34 UTC (permalink / raw) To: gdb-patches From: Simon Marchi <simon.marchi@polymtl.ca> This simplifies the code a bit. gdb/ChangeLog: * psymtab.c (partial_symtab::partial_symtab): Change last_objfile_name to be an std::string. * symfile.c (allocate_symtab): Likewise. Change-Id: I3dfe217233ed9346c2abc04a9b1be0df69a90af8 --- gdb/psymtab.c | 11 +++++------ gdb/symfile.c | 11 +++++------ 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/gdb/psymtab.c b/gdb/psymtab.c index 597817269c1..2ee05ec415b 100644 --- a/gdb/psymtab.c +++ b/gdb/psymtab.c @@ -1575,16 +1575,15 @@ partial_symtab::partial_symtab (const char *filename_, { /* Be a bit clever with debugging messages, and don't print objfile every time, only when it changes. */ - static char *last_objfile_name = NULL; + static std::string last_objfile_name; + const char *this_objfile_name = objfile_name (objfile); - if (last_objfile_name == NULL - || strcmp (last_objfile_name, objfile_name (objfile)) != 0) + if (last_objfile_name.empty () || last_objfile_name != this_objfile_name) { - xfree (last_objfile_name); - last_objfile_name = xstrdup (objfile_name (objfile)); + last_objfile_name = this_objfile_name; fprintf_filtered (gdb_stdlog, "Creating one or more psymtabs for objfile %s ...\n", - last_objfile_name); + this_objfile_name); } fprintf_filtered (gdb_stdlog, "Created psymtab %s for module %s.\n", diff --git a/gdb/symfile.c b/gdb/symfile.c index adcdc169306..ce696321e25 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -2772,16 +2772,15 @@ allocate_symtab (struct compunit_symtab *cust, const char *filename) { /* Be a bit clever with debugging messages, and don't print objfile every time, only when it changes. */ - static char *last_objfile_name = NULL; + static std::string last_objfile_name; + const char *this_objfile_name = objfile_name (objfile); - if (last_objfile_name == NULL - || strcmp (last_objfile_name, objfile_name (objfile)) != 0) + if (last_objfile_name.empty () || last_objfile_name != this_objfile_name) { - xfree (last_objfile_name); - last_objfile_name = xstrdup (objfile_name (objfile)); + last_objfile_name = this_objfile_name; fprintf_filtered (gdb_stdlog, "Creating one or more symtabs for objfile %s ...\n", - last_objfile_name); + this_objfile_name); } fprintf_filtered (gdb_stdlog, "Created symtab %s for module %s.\n", -- 2.30.0 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/4] gdb: use std::string in partial_symtab::partial_symtab / allocate_symtab 2021-03-22 19:34 ` [PATCH 2/4] gdb: use std::string in partial_symtab::partial_symtab / allocate_symtab Simon Marchi @ 2021-04-01 17:43 ` Tom Tromey 0 siblings, 0 replies; 16+ messages in thread From: Tom Tromey @ 2021-04-01 17:43 UTC (permalink / raw) To: Simon Marchi via Gdb-patches; +Cc: Simon Marchi >>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: Simon> From: Simon Marchi <simon.marchi@polymtl.ca> Simon> This simplifies the code a bit. Simon> gdb/ChangeLog: Simon> * psymtab.c (partial_symtab::partial_symtab): Change Simon> last_objfile_name to be an std::string. Simon> * symfile.c (allocate_symtab): Likewise. This looks good to me. Tom ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/4] gdb: pass objfile_per_bfd_storage instead of objfile to partial_symtab 2021-03-22 19:34 [PATCH 0/4] Small Misc psymtabs / objfile cleanups Simon Marchi 2021-03-22 19:34 ` [PATCH 1/4] gdb: add intern method to objfile_per_bfd_storage Simon Marchi 2021-03-22 19:34 ` [PATCH 2/4] gdb: use std::string in partial_symtab::partial_symtab / allocate_symtab Simon Marchi @ 2021-03-22 19:34 ` Simon Marchi 2021-04-01 17:47 ` Tom Tromey 2021-03-22 19:34 ` [PATCH 4/4] gdb: remove objfile parameter from get_objfile_bfd_data Simon Marchi 3 siblings, 1 reply; 16+ messages in thread From: Simon Marchi @ 2021-03-22 19:34 UTC (permalink / raw) To: gdb-patches From: Simon Marchi <simon.marchi@polymtl.ca> Since partial_symtab is supposed to be objfile-independent (since series [1]), I think it would make sense for partial_symtab to not take an objfile as a parameter in its constructor. This patch replaces that parameter with an objfile_per_bfd_storage parameter. The objfile is used for two things: - to get the objfile_name, for debug messages. We can get that name from the bfd instead. - to intern the partial symtab filename. Even though it goes through an objfile method, the request is actually forwarded to the underlying objfile_per_bfd_storage. So we can ask the new objfile_per_bfd_storage instead. In order to get a reference to the BFD from the objfile_per_bfd_storage, the BFD is saved in the objfile_per_bfd_storage object. [1] https://sourceware.org/pipermail/gdb-patches/2021-February/176625.html gdb/ChangeLog: * psympriv.h (struct partial_symtab) <partial_symtab>: Change objfile parameter for objfile_per_bfd_storage, adjust callers. (struct standard_psymtab) <standard_psymtab>: Likewise. (struct legacy_psymtab) <legacy_psymtab>: Likewise. * psymtab.c (partial_symtab::partial_symtab): Likewise. * ctfread.c (struct ctf_psymtab): Likewise. * dwarf2/read.h (struct dwarf2_psymtab): Likewise. * dwarf2/read.c (struct dwarf2_include_psymtab): Likewise. (dwarf2_create_include_psymtab): Likewise. * objfiles.h (struct objfile_per_bfd_storage) <objfile_per_bfd_storage>: Add bfd parameter, adjust callers. <bfd_>: New method. <m_bfd>: New field. * objfiles.c (get_objfile_bfd_data): Change-Id: I2ed3ab5d2e6f27d034bd4dc26ae2fae7b0b8a2b9 --- gdb/ctfread.c | 6 +++--- gdb/dbxread.c | 4 ++-- gdb/dwarf2/read.c | 18 ++++++++---------- gdb/dwarf2/read.h | 4 ++-- gdb/mdebugread.c | 6 +++--- gdb/objfiles.c | 2 +- gdb/objfiles.h | 16 ++++++++++++++-- gdb/psympriv.h | 23 ++++++++++++----------- gdb/psymtab.c | 20 ++++++++++---------- gdb/xcoffread.c | 4 ++-- 10 files changed, 57 insertions(+), 46 deletions(-) diff --git a/gdb/ctfread.c b/gdb/ctfread.c index 616464c77df..fae244d4481 100644 --- a/gdb/ctfread.c +++ b/gdb/ctfread.c @@ -125,9 +125,9 @@ struct ctf_psymtab : public standard_psymtab { ctf_psymtab (const char *filename, psymtab_storage *partial_symtabs, - struct objfile *objfile, + objfile_per_bfd_storage *objfile_per_bfd, CORE_ADDR addr) - : standard_psymtab (filename, partial_symtabs, objfile, addr) + : standard_psymtab (filename, partial_symtabs, objfile_per_bfd, addr) { } @@ -1369,7 +1369,7 @@ create_partial_symtab (const char *name, ctf_psymtab *pst; struct ctf_context *ccx; - pst = new ctf_psymtab (name, partial_symtabs, objfile, 0); + pst = new ctf_psymtab (name, partial_symtabs, objfile->per_bfd, 0); ccx = XOBNEW (&objfile->objfile_obstack, struct ctf_context); ccx->fp = cfp; diff --git a/gdb/dbxread.c b/gdb/dbxread.c index 99a36cafa15..5cf77e9c08a 100644 --- a/gdb/dbxread.c +++ b/gdb/dbxread.c @@ -1917,7 +1917,7 @@ start_psymtab (psymtab_storage *partial_symtabs, struct objfile *objfile, const char *filename, CORE_ADDR textlow, int ldsymoff) { legacy_psymtab *result = new legacy_psymtab (filename, partial_symtabs, - objfile, textlow); + objfile->per_bfd, textlow); result->read_symtab_private = XOBNEW (&objfile->objfile_obstack, struct symloc); @@ -2040,7 +2040,7 @@ dbx_end_psymtab (struct objfile *objfile, psymtab_storage *partial_symtabs, for (i = 0; i < num_includes; i++) { legacy_psymtab *subpst = - new legacy_psymtab (include_list[i], partial_symtabs, objfile); + new legacy_psymtab (include_list[i], partial_symtabs, objfile->per_bfd); subpst->read_symtab_private = XOBNEW (&objfile->objfile_obstack, struct symloc); diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index 2bfb13d6d0e..58b370ecd4a 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -6245,8 +6245,8 @@ struct dwarf2_include_psymtab : public partial_symtab { dwarf2_include_psymtab (const char *filename, psymtab_storage *partial_symtabs, - struct objfile *objfile) - : partial_symtab (filename, partial_symtabs, objfile) + objfile_per_bfd_storage *objfile_per_bfd) + : partial_symtab (filename, partial_symtabs, objfile_per_bfd) { } @@ -6302,10 +6302,10 @@ dwarf2_create_include_psymtab (dwarf2_per_bfd *per_bfd, const char *name, dwarf2_psymtab *pst, psymtab_storage *partial_symtabs, - struct objfile *objfile) + objfile_per_bfd_storage *objfile_per_bfd) { dwarf2_include_psymtab *subpst - = new dwarf2_include_psymtab (name, partial_symtabs, objfile); + = new dwarf2_include_psymtab (name, partial_symtabs, objfile_per_bfd); if (!IS_ABSOLUTE_PATH (subpst->filename)) subpst->dirname = pst->dirname; @@ -7557,11 +7557,9 @@ create_partial_symtab (dwarf2_per_cu_data *per_cu, dwarf2_per_objfile *per_objfile, const char *name) { - struct objfile *objfile = per_objfile->objfile; - dwarf2_psymtab *pst; - - pst = new dwarf2_psymtab (name, per_objfile->per_bfd->partial_symtabs.get (), - objfile, per_cu); + dwarf2_psymtab *pst + = new dwarf2_psymtab (name, per_objfile->per_bfd->partial_symtabs.get (), + per_objfile->objfile->per_bfd, per_cu); pst->psymtabs_addrmap_supported = true; @@ -22008,7 +22006,7 @@ dwarf_decode_lines (struct line_header *lh, const char *comp_dir, dwarf2_create_include_psymtab (cu->per_objfile->per_bfd, include_name, pst, cu->per_objfile->per_bfd->partial_symtabs.get (), - objfile); + objfile->per_bfd); } } else diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h index 82ab3879a5b..416d8eae959 100644 --- a/gdb/dwarf2/read.h +++ b/gdb/dwarf2/read.h @@ -408,9 +408,9 @@ struct dwarf2_psymtab : public partial_symtab { dwarf2_psymtab (const char *filename, psymtab_storage *partial_symtabs, - struct objfile *objfile, + objfile_per_bfd_storage *objfile_per_bfd, dwarf2_per_cu_data *per_cu) - : partial_symtab (filename, partial_symtabs, objfile, 0), + : partial_symtab (filename, partial_symtabs, objfile_per_bfd, 0), per_cu_data (per_cu) { } diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c index 7bf4564aecb..026f2ff20da 100644 --- a/gdb/mdebugread.c +++ b/gdb/mdebugread.c @@ -2605,8 +2605,8 @@ parse_partial_symbols (minimal_symbol_reader &reader, textlow = fh->adr; else textlow = 0; - pst = new legacy_psymtab (fdr_name (fh), partial_symtabs, objfile, - textlow); + pst = new legacy_psymtab (fdr_name (fh), partial_symtabs, + objfile->per_bfd, textlow); pst->read_symtab_private = XOBNEW (&objfile->objfile_obstack, symloc); memset (pst->read_symtab_private, 0, sizeof (struct symloc)); @@ -4646,7 +4646,7 @@ new_psymtab (const char *name, psymtab_storage *partial_symtabs, { legacy_psymtab *psymtab; - psymtab = new legacy_psymtab (name, partial_symtabs, objfile); + psymtab = new legacy_psymtab (name, partial_symtabs, objfile->per_bfd); /* Keep a backpointer to the file's symbols. */ diff --git a/gdb/objfiles.c b/gdb/objfiles.c index ed51c31c8b9..702900761f3 100644 --- a/gdb/objfiles.c +++ b/gdb/objfiles.c @@ -134,7 +134,7 @@ get_objfile_bfd_data (struct objfile *objfile, struct bfd *abfd) if (storage == NULL) { - storage = new objfile_per_bfd_storage; + storage = new objfile_per_bfd_storage (abfd); /* If the object requires gdb to do relocations, we simply fall back to not sharing data across users. These cases are rare enough that this seems reasonable. */ diff --git a/gdb/objfiles.h b/gdb/objfiles.h index d9aa06636f5..e8833794c14 100644 --- a/gdb/objfiles.h +++ b/gdb/objfiles.h @@ -264,8 +264,8 @@ struct minimal_symbol_iterator struct objfile_per_bfd_storage { - objfile_per_bfd_storage () - : minsyms_read (false) + objfile_per_bfd_storage (bfd *bfd) + : minsyms_read (false), m_bfd (bfd) {} ~objfile_per_bfd_storage (); @@ -278,6 +278,13 @@ struct objfile_per_bfd_storage return (const char *) string_cache.insert (str.data (), str.size () + 1); } + /* Get the BFD this object is associated to. */ + + bfd *bfd_ () const + { + return m_bfd; + } + /* The storage has an obstack of its own. */ auto_obstack storage_obstack; @@ -355,6 +362,11 @@ struct objfile_per_bfd_storage /* All the different languages of symbols found in the demangled hash table. */ std::bitset<nr_languages> demangled_hash_languages; + +private: + /* The BFD this object is associated to. */ + + bfd *m_bfd; }; /* An iterator that first returns a parent objfile, and then each diff --git a/gdb/psympriv.h b/gdb/psympriv.h index bbae2fc90e4..f6e7feaf3f4 100644 --- a/gdb/psympriv.h +++ b/gdb/psympriv.h @@ -111,7 +111,8 @@ enum class psymbol_placement struct partial_symtab { - /* Allocate a new partial symbol table associated with OBJFILE. + /* Allocate a new partial symbol table. + FILENAME (which must be non-NULL) is the filename of this partial symbol table; it is copied into the appropriate storage. The partial symtab will also be installed using @@ -119,7 +120,7 @@ struct partial_symtab partial_symtab (const char *filename, psymtab_storage *partial_symtabs, - struct objfile *objfile) + objfile_per_bfd_storage *objfile_per_bfd) ATTRIBUTE_NONNULL (2) ATTRIBUTE_NONNULL (3); /* Like the above, but also sets the initial text low and text high @@ -128,7 +129,7 @@ struct partial_symtab partial_symtab (const char *filename, psymtab_storage *partial_symtabs, - struct objfile *objfile, + objfile_per_bfd_storage *objfile_per_bfd, CORE_ADDR addr) ATTRIBUTE_NONNULL (2) ATTRIBUTE_NONNULL (3); @@ -369,16 +370,16 @@ struct standard_psymtab : public partial_symtab { standard_psymtab (const char *filename, psymtab_storage *partial_symtabs, - struct objfile *objfile) - : partial_symtab (filename, partial_symtabs, objfile) + objfile_per_bfd_storage *objfile_per_bfd) + : partial_symtab (filename, partial_symtabs, objfile_per_bfd) { } standard_psymtab (const char *filename, psymtab_storage *partial_symtabs, - struct objfile *objfile, + objfile_per_bfd_storage *objfile_per_bfd, CORE_ADDR addr) - : partial_symtab (filename, partial_symtabs, objfile, addr) + : partial_symtab (filename, partial_symtabs, objfile_per_bfd, addr) { } @@ -411,16 +412,16 @@ struct legacy_psymtab : public standard_psymtab { legacy_psymtab (const char *filename, psymtab_storage *partial_symtabs, - struct objfile *objfile) - : standard_psymtab (filename, partial_symtabs, objfile) + objfile_per_bfd_storage *objfile_per_bfd) + : standard_psymtab (filename, partial_symtabs, objfile_per_bfd) { } legacy_psymtab (const char *filename, psymtab_storage *partial_symtabs, - struct objfile *objfile, + objfile_per_bfd_storage *objfile_per_bfd, CORE_ADDR addr) - : standard_psymtab (filename, partial_symtabs, objfile, addr) + : standard_psymtab (filename, partial_symtabs, objfile_per_bfd, addr) { } diff --git a/gdb/psymtab.c b/gdb/psymtab.c index 2ee05ec415b..93e2a5c8a01 100644 --- a/gdb/psymtab.c +++ b/gdb/psymtab.c @@ -1437,9 +1437,9 @@ psymbol_functions::find_compunit_symtab_by_address (struct objfile *objfile, partial_symtab::partial_symtab (const char *filename, psymtab_storage *partial_symtabs, - struct objfile *objfile, + objfile_per_bfd_storage *objfile_per_bfd, CORE_ADDR textlow) - : partial_symtab (filename, partial_symtabs, objfile) + : partial_symtab (filename, partial_symtabs, objfile_per_bfd) { set_text_low (textlow); set_text_high (raw_text_low ()); /* default */ @@ -1562,28 +1562,28 @@ partial_symtab::add_psymbol (gdb::string_view name, bool copy_name, partial_symtab::partial_symtab (const char *filename_, psymtab_storage *partial_symtabs, - struct objfile *objfile) + objfile_per_bfd_storage *objfile_per_bfd) : searched_flag (PST_NOT_SEARCHED), text_low_valid (0), text_high_valid (0) { partial_symtabs->install_psymtab (this); - filename = objfile->intern (filename_); + filename = objfile_per_bfd->intern (filename_); if (symtab_create_debug) { /* Be a bit clever with debugging messages, and don't print objfile every time, only when it changes. */ - static std::string last_objfile_name; - const char *this_objfile_name = objfile_name (objfile); + static std::string last_bfd_name; + const char *this_bfd_name = bfd_get_filename (objfile_per_bfd->bfd_ ()); - if (last_objfile_name.empty () || last_objfile_name != this_objfile_name) + if (last_bfd_name.empty () || last_bfd_name != this_bfd_name) { - last_objfile_name = this_objfile_name; + last_bfd_name = this_bfd_name; fprintf_filtered (gdb_stdlog, - "Creating one or more psymtabs for objfile %s ...\n", - this_objfile_name); + "Creating one or more psymtabs for %s ...\n", + this_bfd_name); } fprintf_filtered (gdb_stdlog, "Created psymtab %s for module %s.\n", diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c index 30ac876e8e3..368572797f6 100644 --- a/gdb/xcoffread.c +++ b/gdb/xcoffread.c @@ -1966,7 +1966,7 @@ xcoff_start_psymtab (psymtab_storage *partial_symtabs, { /* We fill in textlow later. */ legacy_psymtab *result = new legacy_psymtab (filename, partial_symtabs, - objfile, 0); + objfile->per_bfd, 0); result->read_symtab_private = XOBNEW (&objfile->objfile_obstack, struct symloc); @@ -2022,7 +2022,7 @@ xcoff_end_psymtab (struct objfile *objfile, psymtab_storage *partial_symtabs, for (i = 0; i < num_includes; i++) { legacy_psymtab *subpst = - new legacy_psymtab (include_list[i], partial_symtabs, objfile); + new legacy_psymtab (include_list[i], partial_symtabs, objfile->per_bfd); subpst->read_symtab_private = XOBNEW (&objfile->objfile_obstack, symloc); ((struct symloc *) subpst->read_symtab_private)->first_symnum = 0; -- 2.30.0 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/4] gdb: pass objfile_per_bfd_storage instead of objfile to partial_symtab 2021-03-22 19:34 ` [PATCH 3/4] gdb: pass objfile_per_bfd_storage instead of objfile to partial_symtab Simon Marchi @ 2021-04-01 17:47 ` Tom Tromey 2021-04-02 15:42 ` Simon Marchi 0 siblings, 1 reply; 16+ messages in thread From: Tom Tromey @ 2021-04-01 17:47 UTC (permalink / raw) To: Simon Marchi via Gdb-patches; +Cc: Simon Marchi >>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: Simon> Since partial_symtab is supposed to be objfile-independent (since series Simon> [1]), I think it would make sense for partial_symtab to not take an Simon> objfile as a parameter in its constructor. Definitely. Thanks for doing this. I had one nit: Simon> + /* Get the BFD this object is associated to. */ Simon> + Simon> + bfd *bfd_ () const Simon> + { Simon> + return m_bfd; Simon> + } This seems like an unusual choice of name for an accessor. Maybe 'get_bfd' instead? Tom ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/4] gdb: pass objfile_per_bfd_storage instead of objfile to partial_symtab 2021-04-01 17:47 ` Tom Tromey @ 2021-04-02 15:42 ` Simon Marchi 0 siblings, 0 replies; 16+ messages in thread From: Simon Marchi @ 2021-04-02 15:42 UTC (permalink / raw) To: Tom Tromey, Simon Marchi via Gdb-patches; +Cc: Simon Marchi On 2021-04-01 1:47 p.m., Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: > > Simon> Since partial_symtab is supposed to be objfile-independent (since series > Simon> [1]), I think it would make sense for partial_symtab to not take an > Simon> objfile as a parameter in its constructor. > > Definitely. Thanks for doing this. > > I had one nit: > > Simon> + /* Get the BFD this object is associated to. */ > Simon> + > Simon> + bfd *bfd_ () const > Simon> + { > Simon> + return m_bfd; > Simon> + } > > This seems like an unusual choice of name for an accessor. > Maybe 'get_bfd' instead? Hmm yeah. I generally like to not use `get_` for getters. For example, just use `foo.name ()` for a getter that gets the name. But I wouldn't let me use `bfd` here. Anyway, I'll push with `get_bfd`. Thanks, Simon ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 4/4] gdb: remove objfile parameter from get_objfile_bfd_data 2021-03-22 19:34 [PATCH 0/4] Small Misc psymtabs / objfile cleanups Simon Marchi ` (2 preceding siblings ...) 2021-03-22 19:34 ` [PATCH 3/4] gdb: pass objfile_per_bfd_storage instead of objfile to partial_symtab Simon Marchi @ 2021-03-22 19:34 ` Simon Marchi 2021-04-01 17:52 ` Tom Tromey 3 siblings, 1 reply; 16+ messages in thread From: Simon Marchi @ 2021-03-22 19:34 UTC (permalink / raw) To: gdb-patches From: Simon Marchi <simon.marchi@polymtl.ca> I noticed it was unused. I think that makes sense, as it shows that objfile_per_bfd_storage is not specific to one objfile (it can be shared by multiple objfiles that have the same bfd). There is one thing I wonder though, maybe I'm missing something. If the BFD doesn't require relocation, get_objfile_bfd_data stores the newly allocated object in objfiles_bfd_data, so we can assume that objfiles_bfd_data is the owner of the object. When the bfd's refcount drops to 0, the corresponding objfile_per_bfd_storage object in objfiles_bfd_data is deleted. But if the BFD requires relocation, get_objfile_bfd_data returns a newly allocated object that isn't kept anywhere else (and isn't shared). So the objfile becomes the owner of the objfile_per_bfd_storage object. In objfile::~objfile, we have this: if (obfd) gdb_bfd_unref (obfd); else delete per_bfd; I'm thinking that obfd could be non-nullptr, and it could require relocation. In that case, it would never be freed. Anyway, that's not really connected to this patch. gdb/ChangeLog: * objfiles.c (get_objfile_bfd_data): Remove objfile parameter, adjust callers. Change-Id: Ifa3158074ea6b42686780ba09d0c964b0cf14cf1 --- gdb/objfiles.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gdb/objfiles.c b/gdb/objfiles.c index 702900761f3..f68e97bd08f 100644 --- a/gdb/objfiles.c +++ b/gdb/objfiles.c @@ -125,7 +125,7 @@ objfile_per_bfd_storage::~objfile_per_bfd_storage () only be called when allocating or re-initializing OBJFILE. */ static struct objfile_per_bfd_storage * -get_objfile_bfd_data (struct objfile *objfile, struct bfd *abfd) +get_objfile_bfd_data (bfd *abfd) { struct objfile_per_bfd_storage *storage = NULL; @@ -154,7 +154,7 @@ get_objfile_bfd_data (struct objfile *objfile, struct bfd *abfd) void set_objfile_per_bfd (struct objfile *objfile) { - objfile->per_bfd = get_objfile_bfd_data (objfile, objfile->obfd); + objfile->per_bfd = get_objfile_bfd_data (objfile->obfd); } /* Set the objfile's per-BFD notion of the "main" name and @@ -363,7 +363,7 @@ objfile::objfile (bfd *abfd, const char *name, objfile_flags flags_) build_objfile_section_table (this); } - per_bfd = get_objfile_bfd_data (this, abfd); + per_bfd = get_objfile_bfd_data (abfd); } /* If there is a valid and known entry point, function fills *ENTRY_P with it -- 2.30.0 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 4/4] gdb: remove objfile parameter from get_objfile_bfd_data 2021-03-22 19:34 ` [PATCH 4/4] gdb: remove objfile parameter from get_objfile_bfd_data Simon Marchi @ 2021-04-01 17:52 ` Tom Tromey 2021-04-02 15:52 ` Simon Marchi 0 siblings, 1 reply; 16+ messages in thread From: Tom Tromey @ 2021-04-01 17:52 UTC (permalink / raw) To: Simon Marchi via Gdb-patches; +Cc: Simon Marchi >>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: Simon> But if the BFD requires relocation, get_objfile_bfd_data returns a newly Simon> allocated object that isn't kept anywhere else (and isn't shared). So Simon> the objfile becomes the owner of the objfile_per_bfd_storage object. In Simon> objfile::~objfile, we have this: Simon> if (obfd) Simon> gdb_bfd_unref (obfd); Simon> else Simon> delete per_bfd; Simon> I'm thinking that obfd could be non-nullptr, and it could require Simon> relocation. In that case, it would never be freed. Anyway, that's not Simon> really connected to this patch. Yes, this seems like a bug. Probably objfile should just have an explicit "owns_per_bfd" boolean somewhere. Simon> gdb/ChangeLog: Simon> * objfiles.c (get_objfile_bfd_data): Remove objfile parameter, Simon> adjust callers. This looks good to me. Thank you. Tom ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 4/4] gdb: remove objfile parameter from get_objfile_bfd_data 2021-04-01 17:52 ` Tom Tromey @ 2021-04-02 15:52 ` Simon Marchi 0 siblings, 0 replies; 16+ messages in thread From: Simon Marchi @ 2021-04-02 15:52 UTC (permalink / raw) To: Tom Tromey, Simon Marchi via Gdb-patches; +Cc: Simon Marchi On 2021-04-01 1:52 p.m., Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: > > Simon> But if the BFD requires relocation, get_objfile_bfd_data returns a newly > Simon> allocated object that isn't kept anywhere else (and isn't shared). So > Simon> the objfile becomes the owner of the objfile_per_bfd_storage object. In > Simon> objfile::~objfile, we have this: > > Simon> if (obfd) > Simon> gdb_bfd_unref (obfd); > Simon> else > Simon> delete per_bfd; > > Simon> I'm thinking that obfd could be non-nullptr, and it could require > Simon> relocation. In that case, it would never be freed. Anyway, that's not > Simon> really connected to this patch. > > Yes, this seems like a bug. Probably objfile should just have an > explicit "owns_per_bfd" boolean somewhere. > > Simon> gdb/ChangeLog: > > Simon> * objfiles.c (get_objfile_bfd_data): Remove objfile parameter, > Simon> adjust callers. > > This looks good to me. Thank you. Thanks, all pushed now. I removed the last sentence of the get_objfile_bfd_data comment, it didn't seem appropriate anymore. Simon ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-04-02 15:52 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-22 19:34 [PATCH 0/4] Small Misc psymtabs / objfile cleanups Simon Marchi 2021-03-22 19:34 ` [PATCH 1/4] gdb: add intern method to objfile_per_bfd_storage Simon Marchi 2021-03-23 14:47 ` Christian Biesinger 2021-03-23 15:48 ` Simon Marchi 2021-03-23 15:53 ` Christian Biesinger 2021-03-23 16:08 ` Simon Marchi 2021-04-01 17:44 ` Tom Tromey 2021-04-02 15:38 ` Simon Marchi 2021-03-22 19:34 ` [PATCH 2/4] gdb: use std::string in partial_symtab::partial_symtab / allocate_symtab Simon Marchi 2021-04-01 17:43 ` Tom Tromey 2021-03-22 19:34 ` [PATCH 3/4] gdb: pass objfile_per_bfd_storage instead of objfile to partial_symtab Simon Marchi 2021-04-01 17:47 ` Tom Tromey 2021-04-02 15:42 ` Simon Marchi 2021-03-22 19:34 ` [PATCH 4/4] gdb: remove objfile parameter from get_objfile_bfd_data Simon Marchi 2021-04-01 17:52 ` Tom Tromey 2021-04-02 15:52 ` Simon Marchi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).