diff mbox

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

Message ID 1470438282-4226-3-git-send-email-daniel.kiper@oracle.com (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Kiper Aug. 5, 2016, 11:04 p.m. UTC
Its visibility is not needed and just pollute symbol table.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 xen/arch/x86/boot/head.S |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andrew Cooper Aug. 9, 2016, 1:24 p.m. UTC | #1
On 06/08/16 00:04, Daniel Kiper wrote:
> Its visibility is not needed and just pollute symbol table.
>
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
>  xen/arch/x86/boot/head.S |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index 85770e8..e34351c 100644
> --- 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.

~Andrew
Jan Beulich Aug. 9, 2016, 1:52 p.m. UTC | #2
>>> 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.

Jan
Andrew Cooper Aug. 9, 2016, 2:09 p.m. UTC | #3
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.

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

~Andrew
Jan Beulich Aug. 9, 2016, 2:30 p.m. UTC | #4
>>> 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
diff mbox

Patch

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 85770e8..e34351c 100644
--- 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:
 
         .section .init.rodata, "a", @progbits
         .align 4