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

[Jeremy Drake]

On Tue, 1 Apr 2025, Jan Beulich wrote:

> 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.

I assume the format string can take %s, so I can use the buf/name buffer
here?  Could also make the 9 a %d so the translated string is the same for
_load_config_used.

> > +#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.)

Is there a preferred way to wrap a long name like:
pe_data (abfd)->pe_opthdr.DataDirectory[PE_LOAD_CONFIG_TABLE].VirtualAddress
or do we just live with that one?

>
> 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?

I was wondering if there was something like that available.  There are too
many structs/members/functions for a casual contributor to know all of
them ;)

> > +	  /* 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.
>
> 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.

I can either: add a remark "If anyone cares about such versions, the size
would need to be overridden for them."  Or, if you can point out the
proper incantation to identify i386 here I can do that.  I confirmed on
Windows 11 that for i386 modules it will accept either 0x40 or the 32-bit
value at the VirtualAddress as the Size, while for x86_64 modules it will
only accept the 32-bit value at the VirtualAddress.