[PATCH v4 1/2] bfd: fill in PE load config directory entry.

[Jan Beulich]

On 01.04.2025 02:30, Jeremy Drake wrote:
> @@ -4558,7 +4560,7 @@ _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"),
> +	    (_("%pB: unable to fill in DataDictionary[9] because _tls_used is not defined correctly"),

The wording change is certainly fine. The unconditional dropping of the first
leading underscore is less so.

> @@ -4573,6 +4575,75 @@ _bfd_XXi_final_link_postscript (bfd * abfd, struct coff_final_link_info *pfinfo)
>  #endif
>      }
> 
> +  h1 = coff_link_hash_lookup (coff_hash_table (info),
> +			      (bfd_get_symbol_leading_char (abfd) != 0
> +			       ? "__load_config_used" : "_load_config_used"),
> +			      false, false, true);
> +  if (h1 != NULL)
> +    {
> +      char data[4];
> +      if ((h1->root.type == bfd_link_hash_defined
> +	   || h1->root.type == bfd_link_hash_defweak)
> +	  && h1->root.u.def.section != NULL
> +	  && h1->root.u.def.section->output_section != NULL)
> +	{
> +	  pe_data (abfd)->pe_opthdr.DataDirectory[PE_LOAD_CONFIG_TABLE].VirtualAddress =
> +	    (h1->root.u.def.value
> +	     + h1->root.u.def.section->output_section->vma
> +	     + h1->root.u.def.section->output_offset
> +	     - pe_data (abfd)->pe_opthdr.ImageBase);
> +
> +	  if (pe_data (abfd)->pe_opthdr.DataDirectory[PE_LOAD_CONFIG_TABLE].VirtualAddress &
> +#if !defined(COFF_WITH_pep) && !defined(COFF_WITH_pex64) && !defined(COFF_WITH_peAArch64) && !defined(COFF_WITH_peLoongArch64) && !defined (COFF_WITH_peRiscV64)

While I understand that there's a similar construct in patch context below, please
don't add further bad examples: The line is far too long. (FTAOD keeping format
strings together, as is the case e.g. in the earlier hunk, is okay, because this
helps grep-ing for such strings. It's a good idea to strive for shortening such
messages then though. In the case above [more of them below in the new code you
add] " because" could be replaced by a simple colon, for example, and "is" could
be dropped altogether.)

However, do we need such a pre-processor conditional here in the first place? Can't
you use ...

> +	      3
> +#else
> +	      7
> +#endif

... struct bfd_arch_info's bits_per_address instead?

> +	     )
> +	    {
> +	      _bfd_error_handler
> +		(_("%pB: unable to fill in DataDictionary[10] because _load_config_used is not properly aligned"),
> +		 abfd);
> +	      result = false;
> +	    }
> +
> +	  /* the size is stored as the first 4 bytes at _load_config_used */
> +	  /* the Microsoft PE format documentation says for compatibility with
> +	     Windows XP and earlier, the size must be 64 for x86 images. */

Please adhere to comment style: Start with a capital. End with a full stop
followed by two blanks. I also see no reason to have two adjacent comments;
what wants saying can the folded in a single one.

> +	  if (bfd_get_section_contents (abfd,
> +		h1->root.u.def.section->output_section, data,
> +		h1->root.u.def.section->output_offset + h1->root.u.def.value,
> +		4))
> +	    {
> +	      pe_data (abfd)->pe_opthdr.DataDirectory[PE_LOAD_CONFIG_TABLE].Size =
> +		bfd_get_32 (abfd, data);
> +
> +	      if (pe_data (abfd)->pe_opthdr.DataDirectory[PE_LOAD_CONFIG_TABLE].Size >
> +		  h1->root.u.def.section->size - h1->root.u.def.value)
> +		{
> +		  _bfd_error_handler
> +		    (_("%pB: unable to fill in DataDictionary[10] because the size is too large for the containing section"),
> +		     abfd);
> +		  result = false;
> +		}
> +	    }
> +	  else
> +	    {
> +	      _bfd_error_handler
> +		(_("%pB: unable to fill in DataDictionary[10] because the size can't be read"),
> +		 abfd);
> +	      result = false;
> +	    }

The latter part of the comment isn't reflected anywhere in the code afaict.
Assuming that's intentional, the comment then also needs wording that way.

Jan