diff mbox series

section-match: Check parent archive name as well

Message ID alpine.LSU.2.20.2306261532320.13548@wotan.suse.de
State Unresolved
Headers
Series section-match: Check parent archive name as well |

Checks

Context Check Description
snail/binutils-gdb-check warning Git am fail log

Commit Message

Michael Matz June 26, 2023, 3:35 p.m. UTC 
  rewriting the section matching routines lost a special case
of matching: section statements of the form

    NAME(section-glob)

normally match against NAME being an object file, but like in
the exclude list we happened to accept archive names as NAME
(undocumented).  The documented way to specify (all) archive members
is by using e.g.

    lib.a:(section-glob)

(that does work also with the prefix tree matcher).

But I intended to not actually change behaviour with the prefix
tree implementation.  So, let's also implement checking against
archive names with a similar FIXME comment we already have in
walk_wild_file_in_exclude_list.
---
Regtesting on Alans targets in progress, on x86_64-linux it finished 
already successfully.  Okay for master?

---
 ld/ldlang.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Comments

Michael Matz June 27, 2023, 2:20 p.m. UTC | #1 
Lame self-reply:

On Mon, 26 Jun 2023, Michael Matz via Binutils wrote:

> Regtesting on Alans targets in progress,

that finished uneventful as well.


Ciao,
Michael.
Nick Clifton June 27, 2023, 3:05 p.m. UTC | #2 
Hi Michael,

> rewriting the section matching routines lost a special case
> of matching: section statements of the form
> 
>      NAME(section-glob)
> 
> normally match against NAME being an object file, but like in
> the exclude list we happened to accept archive names as NAME
> (undocumented).  The documented way to specify (all) archive members
> is by using e.g.
> 
>      lib.a:(section-glob)
> 
> (that does work also with the prefix tree matcher).
> 
> But I intended to not actually change behaviour with the prefix
> tree implementation.  So, let's also implement checking against
> archive names with a similar FIXME comment we already have in
> walk_wild_file_in_exclude_list.

Looks good, but please could also update the documentation so that
this use is explicitly cited as being supported ?

Also would you mind adding a link to your post to this list to
the PR 30590 so that anyone reading that bug report can see how
you fixed it.  Thanks

   https://sourceware.org/bugzilla/show_bug.cgi?id=30590

Cheers
   Nick
Michael Matz June 27, 2023, 4:03 p.m. UTC | #3 
Hello Nick,

On Tue, 27 Jun 2023, Nick Clifton wrote:

> >      NAME(section-glob)
> > 
> > normally match against NAME being an object file, but like in
> > the exclude list we happened to accept archive names as NAME
> > (undocumented).  The documented way to specify (all) archive members
> > is by using e.g.
> > 
> >      lib.a:(section-glob)
> > 
> > (that does work also with the prefix tree matcher).
> > 
> > But I intended to not actually change behaviour with the prefix
> > tree implementation.  So, let's also implement checking against
> > archive names with a similar FIXME comment we already have in
> > walk_wild_file_in_exclude_list.
> 
> Looks good, but please could also update the documentation so that
> this use is explicitly cited as being supported ?

Actually, I'm reluctant to do that and would like to keep this 
undocumented.  We wouldn't be able to fix the FIXMEs in 
walk_wild_file_in_exclude_list anymore if such usage was documented, and 
we have a different (documented) way to specify what the user wanted.

In either case that should be a separate and conscious patch as it 
would introduce (formally) new behaviour.  If you still want me to amend 
the patch with a documentation change, I will, but only on second request 
;-)

> Also would you mind adding a link to your post to this list to
> the PR 30590 so that anyone reading that bug report can see how
> you fixed it.  Thanks
> 
>   https://sourceware.org/bugzilla/show_bug.cgi?id=30590

Oh yes, I haven't yet seen that a bugzilla entry was created for that.
The commit will have it.


Ciao,
Michael.
Nick Clifton June 28, 2023, 8:50 a.m. UTC | #4 
Hi Michael,

>> Looks good, but please could also update the documentation so that
>> this use is explicitly cited as being supported ?
> 
> Actually, I'm reluctant to do that and would like to keep this
> undocumented.  We wouldn't be able to fix the FIXMEs in
> walk_wild_file_in_exclude_list anymore if such usage was documented, and
> we have a different (documented) way to specify what the user wanted.
> 
> In either case that should be a separate and conscious patch as it
> would introduce (formally) new behaviour.  If you still want me to amend
> the patch with a documentation change, I will, but only on second request
> ;-)

Fair enough.


>> Also would you mind adding a link to your post to this list to
>> the PR 30590 so that anyone reading that bug report can see how
>> you fixed it.  Thanks
>>
>>    https://sourceware.org/bugzilla/show_bug.cgi?id=30590
> 
> Oh yes, I haven't yet seen that a bugzilla entry was created for that.
> The commit will have it.

Great - patch approved - please apply.

Cheers
   Nick

PS. A patch to add a test for this bug to the linker testsuite is pre-approved,
   should you feel like writing one ....
diff mbox series

Patch

diff --git a/ld/ldlang.c b/ld/ldlang.c
index 78716f17729..e359a89fcc0 100644
--- a/ld/ldlang.c
+++ b/ld/ldlang.c
@@ -445,8 +445,19 @@  walk_wild_section_match (lang_wild_statement_type *ptr,
 	 about unset local_sym_name (in which case lookup_name simply adds
 	 the input file again).  */
       const char *filename = file->local_sym_name;
-      if (filename == NULL
-	  || filename_cmp (filename, file_spec) != 0)
+      lang_input_statement_type *arch_is;
+      if (filename && filename_cmp (filename, file_spec) == 0)
+	;
+      /* FIXME: see also walk_wild_file_in_exclude_list for why we
+	 also check parents BFD (local_sym_)name to match input statements
+	 with unadorned archive names.  */
+      else if (file->the_bfd
+	       && file->the_bfd->my_archive
+	       && (arch_is = bfd_usrdata (file->the_bfd->my_archive))
+	       && arch_is->local_sym_name
+	       && filename_cmp (arch_is->local_sym_name, file_spec) == 0)
+	;
+      else
 	return;
     }