| Message ID | 1470438282-4226-3-git-send-email-daniel.kiper@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
>>> 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
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
>>> 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 --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
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(-)