Re: [PATCH v4 02/19] x86/boot: remove multiboot1_header_end from symbol table

["Jan Beulich" <JBeulich@suse.com>]

>>> On 09.08.16 at 16:09, <andrew.cooper3@citrix.com> wrote:
> On 09/08/16 14:52, Jan Beulich wrote:
>>>>> On 09.08.16 at 15:24, <andrew.cooper3@citrix.com> wrote:
>>> On 06/08/16 00:04, Daniel Kiper wrote:
>>>> --- a/xen/arch/x86/boot/head.S
>>>> +++ b/xen/arch/x86/boot/head.S
>>>> @@ -32,7 +32,7 @@ multiboot1_header_start:       /*** MULTIBOOT1 HEADER 
> ****/
>>>>          .long   MULTIBOOT_HEADER_FLAGS
>>>>          /* Checksum: must be the negated sum of the first two fields. */
>>>>          .long   -(MULTIBOOT_HEADER_MAGIC + MULTIBOOT_HEADER_FLAGS)
>>>> -multiboot1_header_end:
>>>> +.Lmultiboot1_header_end:
>>> I put this in as a non local symbol for a very good reason, and see no
>>> justification to change it.
>>>
>>> It is very important to be able to distinguish data from opcode in the
>>> disassembly, and one extra global symbol will not break the bank.
>> Well, I was about to commit it, but will hold of now that you object.
>> Nevertheless I disagree, and would like to see the patch go in: If
>> there is code starting past this label, then that code should itself
>> have a label, and any padding between the end label above and the
>> code start label is neither code nor data anyway.
> 
> andrewcoop@andrewcoop:/local/xen.git/xen$ objdump -d xen-syms
> xen-syms:     file format elf64-x86-64
> 
> 
> Disassembly of section .text:
> 
> ffff82d080100000 <_start>:
> ffff82d080100000:       e9 2b d0 19 00          jmpq   ffff82d08029d030
> <__start>
> ffff82d080100005:       0f 1f 00                nopl   (%rax)
> 
> ffff82d080100008 <multiboot1_header_start>:
> ffff82d080100008:       02 b0 ad 1b 03 00       add    0x31bad(%rax),%dh
> ffff82d08010000e:       00 00                   add    %al,(%rax)
> ffff82d080100010:       fb                      sti   
> ffff82d080100011:       4f 52                   rex.WRXB push %r10
> ffff82d080100013:       e4 66                   in     $0x66,%al
> 
> ffff82d080100014 <multiboot1_header_end>:
> ffff82d080100014:       66 66 66 2e 0f 1f 84    data16 data16 nopw
> %cs:0x0(%rax,%rax,1)
> ffff82d08010001b:       00 00 00 00 00
> 
> ffff82d080100020 <__high_start>:
> ffff82d080100020:       0f 01 15 df 1f 20 00    lgdt  
> 0x201fdf(%rip)        # ffff82d080302006 <gdt_descr>
> ffff82d080100027:       b9 00 00 00 00          mov    $0x0,%ecx
> 
> There is padding, so the symbol doesn't overlap, but given that one byte
> at the end of the multiboot header is indistinguishable from the the
> 7-byte nop immediately following it, the lack of multiboot1_header_end
> is very deceptive.

Yet if there weren't any padding, which of the symbols you'd get
displayed would - afaik - be undefined/random.

> Leaving this symbol in does not have any downside, and has significant
> upside in terms of clarity of the disassembled source.

I heavily doubt the "significant". Whether padding gets displayed
as data bytes or NOPs is completely irrelevant. Even worse, if the
header didn't happen to end at an instruction boundary (of what
the disassembler thinks are instructions), the output would likely
be worse to look at.

And I also don't, btw, buy your argument of this one symbol
doesn't matter. If you say this for another few hundred symbols,
the difference does matter. Our symbol table is big enough, I'm
all for getting useless cruft out of it.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel