[PATCH,Testsuite] Add .align 2 for labels on Thumb

[PATCH,Testsuite] Add .align 2 for labels on Thumb

[Yao Qi]

[-- Attachment #1: Type: text/plain, Size: 591 bytes --]

We find test failures in gdb.dwarf2/dw2-ref-missing-frame.exp when
test cases are compiled as thumb.
https://bugs.launchpad.net/gdb-linaro/+bug/615997

In dw2-ref-missing-frame-func.c, it is assumed that address of label
'func_loopfb_start' is equal to address of function func_loopfb.
However, in thumb, the label is 16-bit aligned, while function is
32-bit aligned, so label address may not be equal to function address.

Patch below is to set labels 32-bit aligned.  Tested this patch on
both x86 and armel.  OK to apply?

-- 
Yao Qi
CodeSourcery
yao@codesourcery.com
(650) 331-3385 x739

[-- Attachment #2: align.patch --]
[-- Type: text/x-diff, Size: 950 bytes --]

gdb/testsuite/
2010-08-12  Yao Qi  <yao@codesourcery.com>

	* gdb.dwarf2/dw2-ref-missing-frame-func.c: Add .align 2 for labels
	func_nofb_start and func_loopfb_start, so that address of functions
	is equal to these labels on Thumb.

Index: gdb/testsuite/gdb.dwarf2/dw2-ref-missing-frame-func.c
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.dwarf2/dw2-ref-missing-frame-func.c,v
retrieving revision 1.1
diff -u -r1.1 dw2-ref-missing-frame-func.c
--- gdb/testsuite/gdb.dwarf2/dw2-ref-missing-frame-func.c	25 Jun 2010 15:34:46 -0000	1.1
+++ gdb/testsuite/gdb.dwarf2/dw2-ref-missing-frame-func.c	12 Aug 2010 06:33:51 -0000
@@ -19,6 +19,7 @@
 asm ("cu_text_start:");
 
 asm (".globl func_nofb_start");
+asm (".align 2");
 asm ("func_nofb_start:");
 
 void
@@ -31,6 +32,7 @@
 asm ("func_nofb_end:");
 
 asm (".globl func_loopfb_start");
+asm (".align 2");
 asm ("func_loopfb_start:");
 
 void

Re: [PATCH,Testsuite] Add .align 2 for labels on Thumb

[Mark Kettenis]

> Date: Thu, 12 Aug 2010 16:18:14 +0800
> From: Yao Qi <yao@codesourcery.com>
> Content-Disposition: inline
> 
> We find test failures in gdb.dwarf2/dw2-ref-missing-frame.exp when
> test cases are compiled as thumb.
> https://bugs.launchpad.net/gdb-linaro/+bug/615997
> 
> In dw2-ref-missing-frame-func.c, it is assumed that address of label
> 'func_loopfb_start' is equal to address of function func_loopfb.
> However, in thumb, the label is 16-bit aligned, while function is
> 32-bit aligned, so label address may not be equal to function address.
> 
> Patch below is to set labels 32-bit aligned.  Tested this patch on
> both x86 and armel.  OK to apply?

Unfortunately not.  The .align pseudo-op has different effects on
different architectures.  On most architectures .align 2 actually
means 16-bit alignment, which is certainly not what you want on RISC
architectures that have 32-bit wide instructions.

Re: [PATCH,Testsuite] Add .align 2 for labels on Thumb

[Yao Qi]

On Thu, Aug 12, 2010 at 11:35:51AM +0200, Mark Kettenis wrote:
> > Date: Thu, 12 Aug 2010 16:18:14 +0800
> > From: Yao Qi <yao@codesourcery.com>
> > Content-Disposition: inline
> > 
> > We find test failures in gdb.dwarf2/dw2-ref-missing-frame.exp when
> > test cases are compiled as thumb.
> > https://bugs.launchpad.net/gdb-linaro/+bug/615997
> > 
> > In dw2-ref-missing-frame-func.c, it is assumed that address of label
> > 'func_loopfb_start' is equal to address of function func_loopfb.
> > However, in thumb, the label is 16-bit aligned, while function is
> > 32-bit aligned, so label address may not be equal to function address.
> > 
> > Patch below is to set labels 32-bit aligned.  Tested this patch on
> > both x86 and armel.  OK to apply?
> 
> Unfortunately not.  The .align pseudo-op has different effects on
> different architectures.  On most architectures .align 2 actually
> means 16-bit alignment, which is certainly not what you want on RISC
> architectures that have 32-bit wide instructions.
Thanks for your explanation.

In original test case, breakpoint is set on insn nop, while main
branches to insn push, so breakpoint is not hit.

0000000a <func_loopfb_start>:
   a:   bf00            nop 

0000000c <func_loopfb>:
   c:   b480            push    {r7}

I thought this nop is generated for alignment, so I added '.align 2'
for label func_loopfb_start to force it to be equal to func_loopfb.  I
make a mistake here.

I don't know why nop is generated on label func_loopfb_start, and once
'.align 2' is added, nop is *not* generated, so failures go away by
accident.

-- 
Yao Qi
CodeSourcery
yao@codesourcery.com
(650) 331-3385 x739

Re: [PATCH,Testsuite] Add .align 2 for labels on Thumb

[Daniel Jacobowitz]

On Thu, Aug 12, 2010 at 07:49:08PM +0800, Yao Qi wrote:
> In original test case, breakpoint is set on insn nop, while main
> branches to insn push, so breakpoint is not hit.
> 
> 0000000a <func_loopfb_start>:
>    a:   bf00            nop 
> 
> 0000000c <func_loopfb>:
>    c:   b480            push    {r7}
> 
> I thought this nop is generated for alignment, so I added '.align 2'
> for label func_loopfb_start to force it to be equal to func_loopfb.  I
> make a mistake here.
> 
> I don't know why nop is generated on label func_loopfb_start, and once
> '.align 2' is added, nop is *not* generated, so failures go away by
> accident.

Can you use func_loopfb instead of func_loopfb_start in the .S file?
Or, can you define func_loopfb_start as asm ("func_loopfb_start =
func_loopfb")?

A third option is to address Mark's concern by using ".p2align 4"; I
believe that is 16-byte alignment on all platforms gas supports, and
this test probably requires gas in practice already.

The existing code is not correct, for the reason you've found; the
compiler typically uses .align or .p2align at the start of a function.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH,Testsuite] Add .align 2 for labels on Thumb

[Andreas Schwab]

Daniel Jacobowitz <dan@codesourcery.com> writes:

> Or, can you define func_loopfb_start as asm ("func_loopfb_start =
> func_loopfb")?

That will fail on architectures where the function symbol points to a
function descriptor (powerpc64).

Andreas.

-- 
Andreas Schwab, schwab@redhat.com
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."

Re: [PATCH,Testsuite] Add .align 2 for labels on Thumb

[Yao Qi]

[-- Attachment #1: Type: text/plain, Size: 1402 bytes --]

On Thu, Aug 12, 2010 at 08:34:39AM -0400, Daniel Jacobowitz wrote:
> On Thu, Aug 12, 2010 at 07:49:08PM +0800, Yao Qi wrote:
> > In original test case, breakpoint is set on insn nop, while main
> > branches to insn push, so breakpoint is not hit.
> > 
> > 0000000a <func_loopfb_start>:
> >    a:   bf00            nop 
> > 
> > 0000000c <func_loopfb>:
> >    c:   b480            push    {r7}
> > 
> > I thought this nop is generated for alignment, so I added '.align 2'
> > for label func_loopfb_start to force it to be equal to func_loopfb.  I
> > make a mistake here.
> > 
> > I don't know why nop is generated on label func_loopfb_start, and once
> > '.align 2' is added, nop is *not* generated, so failures go away by
> > accident.
> 
> Can you use func_loopfb instead of func_loopfb_start in the .S file?
> Or, can you define func_loopfb_start as asm ("func_loopfb_start =
> func_loopfb")?
> 
> A third option is to address Mark's concern by using ".p2align 4"; I
> believe that is 16-byte alignment on all platforms gas supports, and
> this test probably requires gas in practice already.

Yeah, update my patch by replacing '.align 2' with '.p2align 4'.
Is that OK?

> 
> The existing code is not correct, for the reason you've found; the
> compiler typically uses .align or .p2align at the start of a function.

I see, thanks.

-- 
Yao Qi
CodeSourcery
yao@codesourcery.com
(650) 331-3385 x739

[-- Attachment #2: align.patch --]
[-- Type: text/x-diff, Size: 840 bytes --]

gdb/testsuite/
2010-08-12  Yao Qi  <yao@codesourcery.com>

	* gdb.dwarf2/dw2-ref-missing-frame-func.c: Add .p2align 4 for labels
	func_nofb_start and func_loopfb_start, so that address of functions
	is equal to these labels on Thumb.

diff --git a/gdb/testsuite/gdb.dwarf2/dw2-ref-missing-frame-func.c b/gdb/testsuite/gdb.dwarf2/dw2-ref-missing-frame-func.c
index 5f77883..e6fa680 100644
--- a/gdb/testsuite/gdb.dwarf2/dw2-ref-missing-frame-func.c
+++ b/gdb/testsuite/gdb.dwarf2/dw2-ref-missing-frame-func.c
@@ -19,6 +19,7 @@ asm (".globl cu_text_start");
 asm ("cu_text_start:");
 
 asm (".globl func_nofb_start");
+asm (".p2align 4");
 asm ("func_nofb_start:");
 
 void
@@ -31,6 +32,7 @@ asm (".globl func_nofb_end");
 asm ("func_nofb_end:");
 
 asm (".globl func_loopfb_start");
+asm (".p2align 4");
 asm ("func_loopfb_start:");
 
 void

Re: [PATCH,Testsuite] Add .align 2 for labels on Thumb

[Daniel Jacobowitz]

On Thu, Aug 12, 2010 at 09:21:55PM +0800, Yao Qi wrote:
> On Thu, Aug 12, 2010 at 08:34:39AM -0400, Daniel Jacobowitz wrote:
> > On Thu, Aug 12, 2010 at 07:49:08PM +0800, Yao Qi wrote:
> > > In original test case, breakpoint is set on insn nop, while main
> > > branches to insn push, so breakpoint is not hit.
> > > 
> > > 0000000a <func_loopfb_start>:
> > >    a:   bf00            nop 
> > > 
> > > 0000000c <func_loopfb>:
> > >    c:   b480            push    {r7}
> > > 
> > > I thought this nop is generated for alignment, so I added '.align 2'
> > > for label func_loopfb_start to force it to be equal to func_loopfb.  I
> > > make a mistake here.
> > > 
> > > I don't know why nop is generated on label func_loopfb_start, and once
> > > '.align 2' is added, nop is *not* generated, so failures go away by
> > > accident.
> > 
> > Can you use func_loopfb instead of func_loopfb_start in the .S file?
> > Or, can you define func_loopfb_start as asm ("func_loopfb_start =
> > func_loopfb")?
> > 
> > A third option is to address Mark's concern by using ".p2align 4"; I
> > believe that is 16-byte alignment on all platforms gas supports, and
> > this test probably requires gas in practice already.
> 
> Yeah, update my patch by replacing '.align 2' with '.p2align 4'.
> Is that OK?

Looks fine to me.  Mark, what do you think?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH,Testsuite] Add .align 2 for labels on Thumb

[Yao Qi]

> > > 
> > > Can you use func_loopfb instead of func_loopfb_start in the .S file?
> > > Or, can you define func_loopfb_start as asm ("func_loopfb_start =
> > > func_loopfb")?
> > > 
> > > A third option is to address Mark's concern by using ".p2align 4"; I
> > > believe that is 16-byte alignment on all platforms gas supports, and
> > > this test probably requires gas in practice already.
> > 
> > Yeah, update my patch by replacing '.align 2' with '.p2align 4'.
> > Is that OK?
> 
> Looks fine to me.  Mark, what do you think?

Mark,
Is this patch OK to you?

-- 
Yao Qi
CodeSourcery
yao@codesourcery.com
(650) 331-3385 x739

Re: [PATCH,Testsuite] Add .align 2 for labels on Thumb

[Mark Kettenis]

> Date: Mon, 16 Aug 2010 23:39:21 +0800
> From: "Yao Qi" <yao@codesourcery.com>
> 
> > > > 
> > > > Can you use func_loopfb instead of func_loopfb_start in the .S file?
> > > > Or, can you define func_loopfb_start as asm ("func_loopfb_start =
> > > > func_loopfb")?
> > > > 
> > > > A third option is to address Mark's concern by using ".p2align 4"; I
> > > > believe that is 16-byte alignment on all platforms gas supports, and
> > > > this test probably requires gas in practice already.
> > > 
> > > Yeah, update my patch by replacing '.align 2' with '.p2align 4'.
> > > Is that OK?
> > 
> > Looks fine to me.  Mark, what do you think?
> 
> Mark,
> Is this patch OK to you?

Sorry, I though I replied to Daniel's message.

Anyway, I'm afraid I don't understand the problem you're trying to
fix, so it's hard to say for me that the diff is ok.  However, I'd say
that 32-bit alignment should be enough; I don't know any architectures
that have instructions longer than 32-bits and care about alignment.

Re: [PATCH,Testsuite] Add .align 2 for labels on Thumb

[Yao Qi]

On Mon, Aug 16, 2010 at 08:18:07PM +0200, Mark Kettenis wrote:
> > Date: Mon, 16 Aug 2010 23:39:21 +0800
> > From: "Yao Qi" <yao@codesourcery.com>
> > 
> > > 
> > > Looks fine to me.  Mark, what do you think?
> > 
> > Mark,
> > Is this patch OK to you?
> 
> Sorry, I though I replied to Daniel's message.
> 
> Anyway, I'm afraid I don't understand the problem you're trying to
> fix, so it's hard to say for me that the diff is ok.  However, I'd say
> that 32-bit alignment should be enough; I don't know any architectures
> that have instructions longer than 32-bits and care about alignment.

Yeah, I agree.  Patch is committed.
http://sourceware.org/ml/gdb-cvs/2010-08/msg00098.html
http://sourceware.org/ml/gdb-cvs/2010-08/msg00099.html

-- 
Yao Qi
CodeSourcery
yao@codesourcery.com
(650) 331-3385 x739