[v2] bfd: fill in PE load config directory entry.
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
This is filled in with the rva of _load_config_used if defined (much
like _tls_used), and the size is the first 32-bit value at that symbol.
Signed-off-by: Jeremy Drake <sourceware-bugzilla@jdrake.com>
---
bfd/peXXigen.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 45 insertions(+), 1 deletion(-)
Comments
在 2025-3-11 02:25, Jeremy Drake 写道:
> This is filled in with the rva of _load_config_used if defined (much
> like _tls_used), and the size is the first 32-bit value at that symbol.
>
> Signed-off-by: Jeremy Drake <sourceware-bugzilla@jdrake.com>
> ---
> bfd/peXXigen.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 45 insertions(+), 1 deletion(-)
>
This produces the correct load config directories on {i686,x86_64}-w64-mingw32.
--
Best regards,
LIU Hao
On 10.03.2025 19:25, Jeremy Drake wrote:
> @@ -4573,6 +4575,48 @@ _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);
I understand the same is used when looking up _tls_used, yet it feels wrong
to assume that bfd_get_symbol_leading_char() can only ever return nil or '_'.
Nevertheless, because of the pre-existing similar code, I'm not going to
insist that this be corrected right here.
> + 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);
> +
> + /* the size is stored as the first 4 bytes at _load_config_used */
> + 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);
> + else
> + {
> + _bfd_error_handler
> + (_("%pB: unable to fill in DataDictionary[10] because the size can't be read"),
> + abfd);
> + result = false;
> + }
> + }
> + else
> + {
> + _bfd_error_handler
> + (_("%pB: unable to fill in DataDictionary[10] because __load_config_used is missing"),
> + abfd);
Message text here suggests this is rather (or also?) meant to be in the outer
if()'s "else", i.e.
> + result = false;
> + }
> + }
... here. Perhaps things want adjusting such that the message is issued in
both cases, e.g.
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
&& ((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))
h1 = NULL;
if (h1 != NULL)
{
...
?
Jan
在 2025-3-11 15:53, Jan Beulich 写道:
> On 10.03.2025 19:25, Jeremy Drake wrote:
>> @@ -4573,6 +4575,48 @@ _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);
>
> I understand the same is used when looking up _tls_used, yet it feels wrong
> to assume that bfd_get_symbol_leading_char() can only ever return nil or '_'.
> Nevertheless, because of the pre-existing similar code, I'm not going to
> insist that this be corrected right here.
The underscore thing is for x86-32, where external variable names are prefixed to make symbols. Symbols
of `__fastcall` and `__vectorcall` functions are not prefixed with underscores, which doesn't apply to
variables.
On x86-64, ARM32 and ARM64, symbols are unprefixed.
--
Best regards,
LIU Hao
On 11.03.2025 09:11, LIU Hao wrote:
> 在 2025-3-11 15:53, Jan Beulich 写道:
>> On 10.03.2025 19:25, Jeremy Drake wrote:
>>> @@ -4573,6 +4575,48 @@ _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);
>>
>> I understand the same is used when looking up _tls_used, yet it feels wrong
>> to assume that bfd_get_symbol_leading_char() can only ever return nil or '_'.
>> Nevertheless, because of the pre-existing similar code, I'm not going to
>> insist that this be corrected right here.
>
> The underscore thing is for x86-32, where external variable names are prefixed to make symbols. Symbols
> of `__fastcall` and `__vectorcall` functions are not prefixed with underscores, which doesn't apply to
> variables.
>
> On x86-64, ARM32 and ARM64, symbols are unprefixed.
I understand what this is for, but BFD specifically has an abstraction allowing
for prefixes other than '_', and this is being undermined by the _tls_used check
as well as this new one (and iirc also elsewhere). It would be nice to not
further spread such badness.
Jan
在 2025-3-11 16:19, Jan Beulich 写道:
> I understand what this is for, but BFD specifically has an abstraction allowing
> for prefixes other than '_', and this is being undermined by the _tls_used check
> as well as this new one (and iirc also elsewhere). It would be nice to not
> further spread such badness.
I think this can be cleaner if it's desired:
```
char var_symbol[64];
var_symbol[0] = bfd_get_symbol_leading_char (abfd);
char* var_name = var_symbol + !!var_symbol[0];
strcpy(var_name, "_tls_used");
h1 = coff_link_hash_lookup (coff_hash_table (info), var_symbol, false, false, true);
// ...
strcpy(var_name, "_load_config_used");
h1 = coff_link_hash_lookup (coff_hash_table (info), var_symbol, false, false, true);
// ...
```
--
Best regards,
LIU Hao
On Tue, 11 Mar 2025, Jan Beulich wrote:
> I understand the same is used when looking up _tls_used, yet it feels wrong
> to assume that bfd_get_symbol_leading_char() can only ever return nil or '_'.
> Nevertheless, because of the pre-existing similar code, I'm not going to
> insist that this be corrected right here.
OK. It sounds like LIU Hao had some ideas in that department, anyway,
presumably for future cleanup
> > + {
> > + _bfd_error_handler
> > + (_("%pB: unable to fill in DataDictionary[10] because __load_config_used is missing"),
> > + abfd);
>
> Message text here suggests this is rather (or also?) meant to be in the outer
> if()'s "else", i.e.
>
> > + result = false;
> > + }
> > + }
>
> .... here. Perhaps things want adjusting such that the message is issued in
> both cases, e.g.
>
> 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
> && ((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))
> h1 = NULL;
> if (h1 != NULL)
> {
> ...
>
> ?
I'm sorry, I was having a hard time understanding what you were
suggesting. I *think* a light bulb just went on, that you are suggesting
issuing an error if h1 is NULL (the outer if). This is not what I
intended: it is not an error if there is no symbol _load_config_used, it
should just not fill in the load config directory. However, if the symbol
does exist, it should be of the correct type and have sufficient data
available at it to read the size. Does this make sense, or am I
misunderstanding either the code or your feedback?
On 11.03.2025 20:28, Jeremy Drake wrote:
> On Tue, 11 Mar 2025, Jan Beulich wrote:
>
>> I understand the same is used when looking up _tls_used, yet it feels wrong
>> to assume that bfd_get_symbol_leading_char() can only ever return nil or '_'.
>> Nevertheless, because of the pre-existing similar code, I'm not going to
>> insist that this be corrected right here.
>
> OK. It sounds like LIU Hao had some ideas in that department, anyway,
> presumably for future cleanup
>
>>> + {
>>> + _bfd_error_handler
>>> + (_("%pB: unable to fill in DataDictionary[10] because __load_config_used is missing"),
>>> + abfd);
>>
>> Message text here suggests this is rather (or also?) meant to be in the outer
>> if()'s "else", i.e.
>>
>>> + result = false;
>>> + }
>>> + }
>>
>> .... here. Perhaps things want adjusting such that the message is issued in
>> both cases, e.g.
>>
>> 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
>> && ((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))
>> h1 = NULL;
>> if (h1 != NULL)
>> {
>> ...
>>
>> ?
>
>
> I'm sorry, I was having a hard time understanding what you were
> suggesting. I *think* a light bulb just went on, that you are suggesting
> issuing an error if h1 is NULL (the outer if). This is not what I
> intended: it is not an error if there is no symbol _load_config_used, it
> should just not fill in the load config directory. However, if the symbol
> does exist, it should be of the correct type and have sufficient data
> available at it to read the size. Does this make sense, or am I
> misunderstanding either the code or your feedback?
Hmm, yes, makes sense. But then why does the diagnostic say "is missing"?
Jan
在 2025-3-12 16:55, Jan Beulich 写道:
> On 11.03.2025 20:28, Jeremy Drake wrote:
>> I'm sorry, I was having a hard time understanding what you were
>> suggesting. I *think* a light bulb just went on, that you are suggesting
>> issuing an error if h1 is NULL (the outer if). This is not what I
>> intended: it is not an error if there is no symbol _load_config_used, it
>> should just not fill in the load config directory. However, if the symbol
>> does exist, it should be of the correct type and have sufficient data
>> available at it to read the size. Does this make sense, or am I
>> misunderstanding either the code or your feedback?
>
> Hmm, yes, makes sense. But then why does the diagnostic say "is missing"?
>
> Jan
I suspect that message was copied from `_tls_used`.
There are two issues in that message: First is that it should have said `_tls_used` which is what should
be defined in source code. Second is that it looks like the error is triggered when the symbol is defined
in an unexpected way, either it's a common (tentative?) definition or it has no output section. So it
should be
(_("%pB: unable to fill in DataDictionary[9] because _tls_used is not defined correctly"),
and likewise for `_load_config_used`.
There's another minor issue I think, which is these symbols cannot be weak.
--
Best regards,
LIU Hao
On Wed, 12 Mar 2025, LIU Hao wrote:
> 在 2025-3-12 16:55, Jan Beulich 写道:
> > On 11.03.2025 20:28, Jeremy Drake wrote:
> > > I'm sorry, I was having a hard time understanding what you were
> > > suggesting. I *think* a light bulb just went on, that you are suggesting
> > > issuing an error if h1 is NULL (the outer if). This is not what I
> > > intended: it is not an error if there is no symbol _load_config_used, it
> > > should just not fill in the load config directory. However, if the symbol
> > > does exist, it should be of the correct type and have sufficient data
> > > available at it to read the size. Does this make sense, or am I
> > > misunderstanding either the code or your feedback?
> >
> > Hmm, yes, makes sense. But then why does the diagnostic say "is missing"?
> >
> > Jan
>
> I suspect that message was copied from `_tls_used`.
Yes, exactly. I just copied the _tls_used block and adjusted to
_load_config_used and the load config directory. Actual
new code is getting the size out of the section, instead of being a
hardcoded size based on 32/64 bit.
>
> There are two issues in that message: First is that it should have said
> `_tls_used` which is what should be defined in source code. Second is that it
> looks like the error is triggered when the symbol is defined in an unexpected
> way, either it's a common (tentative?) definition or it has no output section.
> So it should be
>
> (_("%pB: unable to fill in DataDictionary[9] because _tls_used is not
> defined correctly"),
>
> and likewise for `_load_config_used`.
>
> There's another minor issue I think, which is these symbols cannot be weak.
If there's agreement on a different message I can update and send a new
version of the patch.
On 12.03.2025 21:30, Jeremy Drake wrote:
> On Wed, 12 Mar 2025, LIU Hao wrote:
>> 在 2025-3-12 16:55, Jan Beulich 写道:
>>> On 11.03.2025 20:28, Jeremy Drake wrote:
>>>> I'm sorry, I was having a hard time understanding what you were
>>>> suggesting. I *think* a light bulb just went on, that you are suggesting
>>>> issuing an error if h1 is NULL (the outer if). This is not what I
>>>> intended: it is not an error if there is no symbol _load_config_used, it
>>>> should just not fill in the load config directory. However, if the symbol
>>>> does exist, it should be of the correct type and have sufficient data
>>>> available at it to read the size. Does this make sense, or am I
>>>> misunderstanding either the code or your feedback?
>>>
>>> Hmm, yes, makes sense. But then why does the diagnostic say "is missing"?
>>
>> I suspect that message was copied from `_tls_used`.
>
> Yes, exactly. I just copied the _tls_used block and adjusted to
> _load_config_used and the load config directory. Actual
> new code is getting the size out of the section, instead of being a
> hardcoded size based on 32/64 bit.
>
>>
>> There are two issues in that message: First is that it should have said
>> `_tls_used` which is what should be defined in source code. Second is that it
>> looks like the error is triggered when the symbol is defined in an unexpected
>> way, either it's a common (tentative?) definition or it has no output section.
>> So it should be
>>
>> (_("%pB: unable to fill in DataDictionary[9] because _tls_used is not
>> defined correctly"),
>>
>> and likewise for `_load_config_used`.
>>
>> There's another minor issue I think, which is these symbols cannot be weak.
>
> If there's agreement on a different message I can update and send a new
> version of the patch.
The wording suggested above reads fine to me, fwiw.
Jan
@@ -593,7 +593,7 @@ _bfd_XXi_swap_aouthdr_out (bfd * abfd, void * in, void * out)
struct internal_extra_pe_aouthdr *extra = &pe->pe_opthdr;
PEAOUTHDR *aouthdr_out = (PEAOUTHDR *) out;
bfd_vma sa, fa, ib;
- IMAGE_DATA_DIRECTORY idata2, idata5, tls;
+ IMAGE_DATA_DIRECTORY idata2, idata5, tls, loadcfg;
sa = extra->SectionAlignment;
fa = extra->FileAlignment;
@@ -602,6 +602,7 @@ _bfd_XXi_swap_aouthdr_out (bfd * abfd, void * in, void * out)
idata2 = pe->pe_opthdr.DataDirectory[PE_IMPORT_TABLE];
idata5 = pe->pe_opthdr.DataDirectory[PE_IMPORT_ADDRESS_TABLE];
tls = pe->pe_opthdr.DataDirectory[PE_TLS_TABLE];
+ loadcfg = pe->pe_opthdr.DataDirectory[PE_LOAD_CONFIG_TABLE];
if (aouthdr_in->tsize)
{
@@ -651,6 +652,7 @@ _bfd_XXi_swap_aouthdr_out (bfd * abfd, void * in, void * out)
extra->DataDirectory[PE_IMPORT_TABLE] = idata2;
extra->DataDirectory[PE_IMPORT_ADDRESS_TABLE] = idata5;
extra->DataDirectory[PE_TLS_TABLE] = tls;
+ extra->DataDirectory[PE_LOAD_CONFIG_TABLE] = loadcfg;
if (extra->DataDirectory[PE_IMPORT_TABLE].VirtualAddress == 0)
/* Until other .idata fixes are made (pending patch), the entry for
@@ -4573,6 +4575,48 @@ _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);
+
+ /* the size is stored as the first 4 bytes at _load_config_used */
+ 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);
+ else
+ {
+ _bfd_error_handler
+ (_("%pB: unable to fill in DataDictionary[10] because the size can't be read"),
+ abfd);
+ result = false;
+ }
+ }
+ else
+ {
+ _bfd_error_handler
+ (_("%pB: unable to fill in DataDictionary[10] because __load_config_used is missing"),
+ abfd);
+ result = false;
+ }
+ }
+
/* If there is a .pdata section and we have linked pdata finally, we
need to sort the entries ascending. */
#if !defined(COFF_WITH_pep) && (defined(COFF_WITH_pex64) || defined(COFF_WITH_peAArch64) || defined(COFF_WITH_peLoongArch64) || defined (COFF_WITH_peRiscV64))