From: Jan Beulich <jbeulich@suse.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wl@xen.org>
Subject: Re: [PATCH 7/8] x86/EFI: keep debug info in xen.efi
Date: Fri, 23 Apr 2021 10:51:40 +0200 [thread overview]
Message-ID: <32bba278-12cc-d105-7b95-6a788992d70c@suse.com> (raw)
In-Reply-To: <YIJ3icwAoGEW/AO+@Air-de-Roger>
On 23.04.2021 09:30, Roger Pau Monné wrote:
> On Thu, Apr 22, 2021 at 06:01:06PM +0200, Jan Beulich wrote:
>> On 22.04.2021 17:53, Roger Pau Monné wrote:
>>> On Thu, Apr 22, 2021 at 05:46:28PM +0200, Jan Beulich wrote:
>>>> On 22.04.2021 16:56, Roger Pau Monné wrote:
>>>>> On Thu, Apr 22, 2021 at 01:03:13PM +0200, Jan Beulich wrote:
>>>>>> On 22.04.2021 10:14, Roger Pau Monné wrote:
>>>>>>> On Wed, Apr 21, 2021 at 05:38:42PM +0200, Jan Beulich wrote:
>>>>>>>> On 21.04.2021 17:30, Roger Pau Monné wrote:
>>>>>>>>> On Wed, Apr 21, 2021 at 03:06:36PM +0200, Jan Beulich wrote:
>>>>>>>>>> On 21.04.2021 13:15, Roger Pau Monné wrote:
>>>>>>>>>>> On Thu, Apr 01, 2021 at 11:47:03AM +0200, Jan Beulich wrote:
>>>>>>>>>>>> --- a/xen/arch/x86/xen.lds.S
>>>>>>>>>>>> +++ b/xen/arch/x86/xen.lds.S
>>>>>>>>>>>> @@ -312,10 +312,60 @@ SECTIONS
>>>>>>>>>>>> *(.reloc)
>>>>>>>>>>>> __base_relocs_end = .;
>>>>>>>>>>>> }
>>>>>>>>>>>> - /* Trick the linker into setting the image size to exactly 16Mb. */
>>>>>>>>>>>> - . = ALIGN(__section_alignment__);
>>>>>>>>>>>> - DECL_SECTION(.pad) {
>>>>>>>>>>>> - . = ALIGN(MB(16));
>>>>>>>>>>>> + .debug_abbrev ALIGN(1) (NOLOAD) : {
>>>>>>>>>>>> + *(.debug_abbrev)
>>>>>>>>>>>> + }
>>>>>>>>>>>> + .debug_info ALIGN(1) (NOLOAD) : {
>>>>>>>>>>>> + *(.debug_info)
>>>>>>>>>>>> + *(.gnu.linkonce.wi.*)
>>>>>>>>>>>> + }
>>>>>>>>>>>> + .debug_types ALIGN(1) (NOLOAD) : {
>>>>>>>>>>>> + *(.debug_types)
>>>>>>>>>>>> + }
>>>>>>>>>>>> + .debug_str ALIGN(1) (NOLOAD) : {
>>>>>>>>>>>> + *(.debug_str)
>>>>>>>>>>>> + }
>>>>>>>>>>>> + .debug_line ALIGN(1) (NOLOAD) : {
>>>>>>>>>>>> + *(.debug_line)
>>>>>>>>>>>> + *(.debug_line.*)
>>>>>>>>>>>> + }
>>>>>>>>>>>> + .debug_line_str ALIGN(1) (NOLOAD) : {
>>>>>>>>>>>> + *(.debug_line_str)
>>>>>>>>>>>> + }
>>>>>>>>>>>> + .debug_names ALIGN(4) (NOLOAD) : {
>>>>>>>>>>>> + *(.debug_names)
>>>>>>>>>>>> + }
>>>>>>>>>>>> + .debug_frame ALIGN(4) (NOLOAD) : {
>>>>>>>>>>>> + *(.debug_frame)
>>>>>>>>>>>> + }
>>>>>>>>>>>> + .debug_loc ALIGN(1) (NOLOAD) : {
>>>>>>>>>>>> + *(.debug_loc)
>>>>>>>>>>>> + }
>>>>>>>>>>>> + .debug_loclists ALIGN(4) (NOLOAD) : {
>>>>>>>>>>>> + *(.debug_loclists)
>>>>>>>>>>>> + }
>>>>>>>>>>>> + .debug_ranges ALIGN(8) (NOLOAD) : {
>>>>>>>>>>>> + *(.debug_ranges)
>>>>>>>>>>>> + }
>>>>>>>>>>>> + .debug_rnglists ALIGN(4) (NOLOAD) : {
>>>>>>>>>>>> + *(.debug_rnglists)
>>>>>>>>>>>> + }
>>>>>>>>>>>> + .debug_addr ALIGN(8) (NOLOAD) : {
>>>>>>>>>>>> + *(.debug_addr)
>>>>>>>>>>>> + }
>>>>>>>>>>>> + .debug_aranges ALIGN(1) (NOLOAD) : {
>>>>>>>>>>>> + *(.debug_aranges)
>>>>>>>>>>>> + }
>>>>>>>>>>>> + .debug_pubnames ALIGN(1) (NOLOAD) : {
>>>>>>>>>>>> + *(.debug_pubnames)
>>>>>>>>>>>> + }
>>>>>>>>>>>> + .debug_pubtypes ALIGN(1) (NOLOAD) : {
>>>>>>>>>>>> + *(.debug_pubtypes)
>>>>>>>>>>>> + }
>>>>>>>>>>>> + /* Trick the linker into setting the image size to no less than 16Mb. */
>>>>>>>>>>>> + __image_end__ = .;
>>>>>>>>>>>> + .pad ALIGN(__section_alignment__) : {
>>>>>>>>>>>> + . = __image_end__ < __image_base__ + MB(16) ? ALIGN(MB(16)) : .;
>>>>>>>>>>>
>>>>>>>>>>> I think this is inside an ifdef EFI region, since this is DWARF info
>>>>>>>>>>> couldn't it also be present when building with EFI disabled?
>>>>>>>>>>
>>>>>>>>>> Of course (and it's not just "could" but "will"), yet the linker will
>>>>>>>>>> do fine (and perhaps even better) without when building ELF. Also
>>>>>>>>>> note that we'll be responsible for keeping the list of sections up-to-
>>>>>>>>>> date. The linker will recognize Dwarf sections by looking for a
>>>>>>>>>> .debug_ prefix. We can't use such here (or at least I'm not aware of
>>>>>>>>>> a suitable mechanism); .debug_* would mean munging together all the
>>>>>>>>>> different kinds of Dwarf sections. Hence by limiting the explicit
>>>>>>>>>> enumeration to PE, I'm trying to avoid anomalies in ELF down the road.
>>>>>>>>>
>>>>>>>>> Right, so we will have to keep this list of debug_ sections updated
>>>>>>>>> manually if/when more of those appear as part of DWARF updates?
>>>>>>>>
>>>>>>>> Yes.
>>>>>>>>
>>>>>>>>> Do we have a way to get some kind of warning or error when a new
>>>>>>>>> section not explicitly handled here appears?
>>>>>>>>
>>>>>>>> ld 2.37 will start warning about such sections, as they'd land at
>>>>>>>> VA 0 and hence below image base.
>>>>>>>
>>>>>>> That seems like a bug in ld?
>>>>>>>
>>>>>>> The '--image-base' option description mentions: "This is the lowest
>>>>>>> memory location that will be used when your program or dll is
>>>>>>> loaded.", so I would expect that if the option is used the default VA
>>>>>>> should be >= image-base, or else the description of the option is not
>>>>>>> consistent, as ld will still place sections at addresses below
>>>>>>> image-base.
>>>>>>
>>>>>> ld's "general" logic is pretty ELF-centric. Hence debugging sections
>>>>>> get placed at VA 0 by default, not matter what the (PE-specific)
>>>>>> --image-base says. Whether that's a bug though I'm not sure: There
>>>>>> are no really good alternatives that could be used by default. Doing
>>>>>> what we do here (and what e.g. Cygwin does) via linker script may not
>>>>>> be appropriate in the common case. In particular it is not generally
>>>>>> correct for debug info to be part of what gets loaded into memory.
>>>>>
>>>>> So with this change here you placate the warnings from newer ld about
>>>>> having a VA < image base,
>>>>
>>>> It's not just about silencing the warnings. The resulting image is
>>>> unusable when the sections don't get placed at a suitable VA.
>>>
>>> And this wasn't an issue before because the linker won't even attempt
>>> to place DWARF sections into a PE output.
>>
>> No, this wasn't an issue before since, for things to work, we
>> simply had to uniformly strip debug info when linking xen.efi. And
>> this is what Andrew said should change. I was initially opposed,
>> until I saw that Cygwin does just this as well.
>
> Just for my own education, do you have a reference about this way of
> packaging debug data by Cygwin?
I've simply built a test program and looked at the binary. The best
reference I could think of is their default linker script in binutils
(ld/scripttempl/pep.sc).
> I've found:
>
> https://cygwin.com/pipermail/cygwin/2003-January/090110.html
>
> Which mentions not setting the ALLOC flag on the debug sections in
> order to prevent them from being loaded. I'm however not able to
> figure out which flag is that on the PE spec, or whether it can be set
> from the linker script.
There's no truly corresponding flag in COFF.
>>>>> but the end result is that now the debug
>>>>> sections will also get loaded when booted from the EFI loader?
>>>>> (because the NOLOAD doesn't have any effect with PE)
>>>>
>>>> Yes. I currently see no other way to retain debug info in xen.efi.
>>>> But to be clear, the memory debug info occupies isn't lost - we
>>>> still free space from _end (or alike) onwards. .reloc, for example,
>>>> also lives there. And I was wondering whether we shouldn't keep
>>>> .comment this way as well.
>>>
>>> Yes, I already realized all this is past _end.
>>>
>>> I wonder however if the use of (NOLOAD) makes all this more confusing,
>>> such sections should only be present on the linker script used for the
>>> PE output, and then the (NOLOAD) doesn't make sense there?
>>>
>>> If so, I think the (NOLOAD) directive should be dropped, and a comment
>>> noting that the debug sections need to be manually added to the PE
>>> output in order to avoid them being placed at VA 0 would be helpful
>>> IMO, likely also mentioning that they would be loaded but discarded
>>> afterwards by Xen because they are all past _end.
>>
>> Earlier on (another sub-thread, maybe) I think I've already said that
>> I'd like to keep (NOLOAD) both for documentation purposes and just in
>> case the linker develops some smarts to actually translate it into
>> anything sensible when linking PE. This is quite different from
>> .reloc, after all - that section has to be loaded for our re-
>> relocation to work correctly. Hence that section not having (NOLOAD)
>> and the debugging sections having it points out the difference.
>
> Sure, that's all fine. I think a comment could be appropriate here, to
> note both that NOLOAD is likely useless and just used for
> documentation purposes, and to also mention the sections needs to be
> explicitly placed in the PE linker script so they are not set at VA 0.
>
> /*
> * Explicitly list debug section for the PE output so that they don't
> * end up at VA 0 which is below image base and thus invalid. Also use
> * the NOLOAD directive, even when currently ignored by PE output, in
> * order to note those sections shouldn't be loaded into memory.
> *
> * Note such sections are past _end, so if loaded will be discarded by
> * Xen anyway.
> */
>
> Feel free to reword or expand the comment.
Yes, I've edited it some while inserting. Will see to get to
submitting v2 then.
> Not sure there's some
> reference we could add here about how debug sections are placed in PE
> files usually.
As said before - I don't think there's any "usually" here, which is
why different environments have invented different ways. The debug
info native to COFF is more like ELF's symbol table (with a little
bit of extra information) plus Dwarf's .debug_line, but not really
fully covering what you'd expect from debug info.
Jan
next prev parent reply other threads:[~2021-04-23 8:52 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-01 9:43 [PATCH 0/8] x86/EFI: build adjustments Jan Beulich
2021-04-01 9:44 ` [PATCH 1/8] x86/EFI: drop stale section special casing when generating base relocs Jan Beulich
2021-04-01 11:51 ` Andrew Cooper
2021-04-01 9:44 ` [PATCH 2/8] x86/EFI: sections may not live at VA 0 in PE binaries Jan Beulich
2021-04-01 12:01 ` Andrew Cooper
2021-04-01 13:51 ` Jan Beulich
2021-04-21 8:52 ` Roger Pau Monné
2021-04-21 10:32 ` Jan Beulich
2021-04-21 12:57 ` Roger Pau Monné
2021-04-21 13:28 ` Jan Beulich
2021-04-01 9:45 ` [PATCH 3/8] x86/EFI: program headers are an ELF concept Jan Beulich
2021-04-21 9:11 ` Roger Pau Monné
2021-04-21 10:36 ` Jan Beulich
2021-04-21 14:21 ` Roger Pau Monné
2021-04-21 14:30 ` Jan Beulich
2021-04-01 9:45 ` [PATCH 4/8] x86/EFI: redo .reloc section bounds determination Jan Beulich
2021-04-21 9:46 ` Roger Pau Monné
2021-04-21 10:44 ` Jan Beulich
2021-04-21 14:54 ` Roger Pau Monné
2021-04-01 9:46 ` [PATCH 5/8] x86: drop use of prelink-efi.o Jan Beulich
2021-04-21 9:51 ` Roger Pau Monné
2021-04-01 9:46 ` [PATCH 6/8] x86/EFI: avoid use of GNU ld's --disable-reloc-section when possible Jan Beulich
2021-04-21 10:21 ` Roger Pau Monné
2021-04-21 12:03 ` Jan Beulich
2021-04-21 15:20 ` Roger Pau Monné
2021-04-21 15:34 ` Jan Beulich
2021-04-22 7:22 ` Roger Pau Monné
2021-04-22 10:42 ` Jan Beulich
2021-04-01 9:47 ` [PATCH 7/8] x86/EFI: keep debug info in xen.efi Jan Beulich
2021-04-21 11:15 ` Roger Pau Monné
2021-04-21 13:06 ` Jan Beulich
2021-04-21 15:30 ` Roger Pau Monné
2021-04-21 15:38 ` Jan Beulich
2021-04-22 8:14 ` Roger Pau Monné
2021-04-22 11:03 ` Jan Beulich
2021-04-22 14:56 ` Roger Pau Monné
2021-04-22 15:46 ` Jan Beulich
2021-04-22 15:53 ` Roger Pau Monné
2021-04-22 16:01 ` Jan Beulich
2021-04-23 7:30 ` Roger Pau Monné
2021-04-23 8:51 ` Jan Beulich [this message]
2021-04-23 10:07 ` Roger Pau Monné
2021-04-23 10:45 ` Jan Beulich
2021-04-23 10:58 ` Roger Pau Monné
2021-04-01 9:47 ` [PATCH 8/8] x86/EFI: don't have an overly large image size Jan Beulich
2021-04-21 11:18 ` Roger Pau Monné
2021-04-21 13:15 ` Jan Beulich
2021-04-15 9:53 ` Ping: [PATCH 0/8] x86/EFI: build adjustments Jan Beulich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=32bba278-12cc-d105-7b95-6a788992d70c@suse.com \
--to=jbeulich@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=roger.pau@citrix.com \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).