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

[Jan Beulich]

On 01.04.2025 20:01, Jeremy Drake wrote:
> 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?

I think so; see e.g. _bfd_XXi_swap_scnhdr_out().

>  Could also make the 9 a %d so the translated string is the same for
> _load_config_used.

Ah yes, that would be nice as well.

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

I probably wouldn't insist on such being wrapped, but it can be, e.g.

    pe_data (abfd)->pe_opthdr.DataDirectory[PE_LOAD_CONFIG_TABLE]
      .VirtualAddress

i.e. continued line indented by one more level, and operator first on the
continued line.

>> 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 ;)

Indeed. I, too, learned of this relatively recently only.

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

This or anything substantially similar would be fine with me. If I was to
re-word that part, I'd go with

"While the Microsoft PE format documentation says that, for compatibility with
 Windows XP and earlier, the size must be 64 for x86 images, we don't check or
 enforce this here."

Jan

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