diff mbox series

[v6,1/3] bfd: properly use bfd_get_symbol_leading_char in peXXigen.

Message ID 1aa77fd4-9b23-da47-6973-84b1f10d1184@jdrake.com
State New
Headers
Series [v6,1/3] bfd: properly use bfd_get_symbol_leading_char in peXXigen. |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Jeremy Drake April 3, 2025, 6:11 p.m. UTC 
  This function returns the leading char to use, so we cannot just assume
it will always be '_' or '\0'.

After review, rationalize the error messages in
_bfd_XXi_final_link_postscript.  They now all correctly refer to
DataDirectory instead of DataDictionary, and use format strings so they
only need one translation.

Signed-off-by: Jeremy Drake <sourceware-bugzilla@jdrake.com>
---
 bfd/peXXigen.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

Comments

Jan Beulich April 4, 2025, 6:54 a.m. UTC | #1 
On 03.04.2025 20:11, Jeremy Drake wrote:
> This function returns the leading char to use, so we cannot just assume
> it will always be '_' or '\0'.
> 
> After review, rationalize the error messages in
> _bfd_XXi_final_link_postscript.  They now all correctly refer to
> DataDirectory instead of DataDictionary, and use format strings so they
> only need one translation.

With that last aspect you went a little too far:

> @@ -4430,8 +4431,8 @@ _bfd_XXi_final_link_postscript (bfd * abfd, struct coff_final_link_info *pfinfo)
>        else
>  	{
>  	  _bfd_error_handler
> -	    (_("%pB: unable to fill in DataDictionary[1] because .idata$2 is missing"),
> -	     abfd);
> +	    (_("%pB: unable to fill in DataDirectory[%d]: %s not defined correctly"),
> +	     abfd, PE_IMPORT_TABLE, ".idata$2");

"is missing" is more precise than "not defined correctly", or at least more
helpful to the user. It's a section after all, not a symbol. Same for the
other .idata$<n> messages then, except for one (see below).

> @@ -4533,17 +4534,16 @@ _bfd_XXi_final_link_postscript (bfd * abfd, struct coff_final_link_info *pfinfo)
>  	  else
>  	    {
>  	      _bfd_error_handler
> -		(_("%pB: unable to fill in DataDictionary[PE_IMPORT_ADDRESS_TABLE(12)]"
> -		   " because .idata$6 is missing"), abfd);
> +		(_("%pB: unable to fill in DataDirectory[%d]: %s not defined correctly"),
> +		 abfd, PE_IMPORT_ADDRESS_TABLE, "__IAT_end__");

This one I'm fine with. Still none of the above really belong in a patch under
the given title, so I'd like to ask that we split things, keeping right here
only what's further down (as it was before).

Judging from earlier commits of yours it looks like you may not have commit
rights. I'd be fine making said adjustments while committing on your behalf.

Jan
Jeremy Drake April 4, 2025, 4:54 p.m. UTC | #2 
On Fri, 4 Apr 2025, Jan Beulich wrote:

> With that last aspect you went a little too far:
>
> > @@ -4430,8 +4431,8 @@ _bfd_XXi_final_link_postscript (bfd * abfd, struct coff_final_link_info *pfinfo)
> >        else
> >  	{
> >  	  _bfd_error_handler
> > -	    (_("%pB: unable to fill in DataDictionary[1] because .idata$2 is missing"),
> > -	     abfd);
> > +	    (_("%pB: unable to fill in DataDirectory[%d]: %s not defined correctly"),
> > +	     abfd, PE_IMPORT_TABLE, ".idata$2");
>
> "is missing" is more precise than "not defined correctly", or at least more
> helpful to the user. It's a section after all, not a symbol. Same for the
> other .idata$<n> messages then, except for one (see below).

> This one I'm fine with. Still none of the above really belong in a patch under
> the given title, so I'd like to ask that we split things, keeping right here
> only what's further down (as it was before).
>
> Judging from earlier commits of yours it looks like you may not have commit
> rights. I'd be fine making said adjustments while committing on your behalf.

Sorry about that.  When you mentioned changing DataDictionary to
DataDirectory, I did a search and found several messages that were the
same.

Correct, I do not have rights to push.  I'd be fine with you making
whatever adjustments (such as adding a Reviewed-by if this is something
this project does) and pushing.  Thanks.
diff mbox series

Patch

diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
index 29012688adf..1401bd73973 100644
--- a/bfd/peXXigen.c
+++ b/bfd/peXXigen.c
@@ -4403,6 +4403,7 @@  _bfd_XXi_final_link_postscript (bfd * abfd, struct coff_final_link_info *pfinfo)
   struct coff_link_hash_entry *h1;
   struct bfd_link_info *info = pfinfo->info;
   bool result = true;
+  char name[20];

   /* There are a few fields that need to be filled in now while we
      have symbol table access.
@@ -4430,8 +4431,8 @@  _bfd_XXi_final_link_postscript (bfd * abfd, struct coff_final_link_info *pfinfo)
       else
 	{
 	  _bfd_error_handler
-	    (_("%pB: unable to fill in DataDictionary[1] because .idata$2 is missing"),
-	     abfd);
+	    (_("%pB: unable to fill in DataDirectory[%d]: %s not defined correctly"),
+	     abfd, PE_IMPORT_TABLE, ".idata$2");
 	  result = false;
 	}

@@ -4450,8 +4451,8 @@  _bfd_XXi_final_link_postscript (bfd * abfd, struct coff_final_link_info *pfinfo)
       else
 	{
 	  _bfd_error_handler
-	    (_("%pB: unable to fill in DataDictionary[1] because .idata$4 is missing"),
-	     abfd);
+	    (_("%pB: unable to fill in DataDirectory[%d]: %s not defined correctly"),
+	     abfd, PE_IMPORT_TABLE, ".idata$4");
 	  result = false;
 	}

@@ -4471,8 +4472,8 @@  _bfd_XXi_final_link_postscript (bfd * abfd, struct coff_final_link_info *pfinfo)
       else
 	{
 	  _bfd_error_handler
-	    (_("%pB: unable to fill in DataDictionary[12] because .idata$5 is missing"),
-	     abfd);
+	    (_("%pB: unable to fill in DataDirectory[%d]: %s not defined correctly"),
+	     abfd, PE_IMPORT_ADDRESS_TABLE, ".idata$5");
 	  result = false;
 	}

@@ -4491,8 +4492,8 @@  _bfd_XXi_final_link_postscript (bfd * abfd, struct coff_final_link_info *pfinfo)
       else
 	{
 	  _bfd_error_handler
-	    (_("%pB: unable to fill in DataDictionary[PE_IMPORT_ADDRESS_TABLE (12)] because .idata$6 is missing"),
-	     abfd);
+	    (_("%pB: unable to fill in DataDirectory[%d]: %s not defined correctly"),
+	     abfd, PE_IMPORT_ADDRESS_TABLE, ".idata$6");
 	  result = false;
 	}
     }
@@ -4533,17 +4534,16 @@  _bfd_XXi_final_link_postscript (bfd * abfd, struct coff_final_link_info *pfinfo)
 	  else
 	    {
 	      _bfd_error_handler
-		(_("%pB: unable to fill in DataDictionary[PE_IMPORT_ADDRESS_TABLE(12)]"
-		   " because .idata$6 is missing"), abfd);
+		(_("%pB: unable to fill in DataDirectory[%d]: %s not defined correctly"),
+		 abfd, PE_IMPORT_ADDRESS_TABLE, "__IAT_end__");
 	      result = false;
 	    }
 	}
     }

-  h1 = coff_link_hash_lookup (coff_hash_table (info),
-			      (bfd_get_symbol_leading_char (abfd) != 0
-			       ? "__tls_used" : "_tls_used"),
-			      false, false, true);
+  name[0] = bfd_get_symbol_leading_char (abfd);
+  strcpy (name + !!name[0], "_tls_used");
+  h1 = coff_link_hash_lookup (coff_hash_table (info), name, false, false, true);
   if (h1 != NULL)
     {
       if ((h1->root.type == bfd_link_hash_defined
@@ -4558,8 +4558,8 @@  _bfd_XXi_final_link_postscript (bfd * abfd, struct coff_final_link_info *pfinfo)
       else
 	{
 	  _bfd_error_handler
-	    (_("%pB: unable to fill in DataDictionary[9] because __tls_used is missing"),
-	     abfd);
+	    (_("%pB: unable to fill in DataDirectory[%d]: %s not defined correctly"),
+	     abfd, PE_TLS_TABLE, name);
 	  result = false;
 	}
      /* According to PECOFF sepcifications by Microsoft version 8.2