RFC: ARM: Support for VFP/NEON registers in coredumps

RFC: ARM: Support for VFP/NEON registers in coredumps

[Dave Martin]

Hi all,

Considering how to add the VFP/NEON register state to Coredumps,
there's no single obvious solution, so I'd like to put this out
for people's comments.

Currently, the coredump consists of several notes, the interesting
ones currently being:

 * NT_PRSTATUS - general process status, including the integer registers
 * NT_FPREGSET - the FPA registers (only present if FPA is in use)

These notes are duplicated per thread when a multithreaded process
dumps core.

There are few options I see:

 a) Dump the VFP/NEON state in NT_FPREGSET.
    This could be appended to, or in place of the legacy FPA regs,
    with flags in a header or (struct elf_prstatus).pr_fpvalid
    to describe what was dumped.  Old gdb etc. may read the
    resulting information as random FPA register contents;
    this could perhaps be avoided by always dumping a dummy
    FPA regs structure at the start of this note, though this
    seems wasteful.

    Possibly, gdb may explode if the NT_FPREGSET note is larger
    then expected.  I've not investigated this.

 b) Dump the VFP/NEON regs in a note of type NT_PRXFPREG.
    This avoids defining a new note type; however, this note type
    is really for x86 -- so overloading it in this way may just
    be nonsensical / confusing.

 c) Create a new note type specially for this, and dump the
    VFP/NEON regs in there.  This is closest to the model followed
    by ptrace, where PTRACE_GETREGS, PTRACE_GETFPREGS and
    PTRAGE_GETVFPREGS are all distinct.


Going for (c) seems sanest to me, and also means that old gdb
will just ignore the new data; this is probably what we want.

Comments are welcome.

Cheers
---Dave

RFC: ARM: Support for VFP/NEON registers in coredumps

[Mikael Pettersson]

Dave Martin writes:
 > Hi all,
 > 
 > Considering how to add the VFP/NEON register state to Coredumps,
 > there's no single obvious solution, so I'd like to put this out
 > for people's comments.
 > 
 > Currently, the coredump consists of several notes, the interesting
 > ones currently being:
 > 
 >  * NT_PRSTATUS - general process status, including the integer registers
 >  * NT_FPREGSET - the FPA registers (only present if FPA is in use)
 > 
 > These notes are duplicated per thread when a multithreaded process
 > dumps core.
 > 
 > There are few options I see:
 > 
 >  a) Dump the VFP/NEON state in NT_FPREGSET.
 >     This could be appended to, or in place of the legacy FPA regs,
 >     with flags in a header or (struct elf_prstatus).pr_fpvalid
 >     to describe what was dumped.  Old gdb etc. may read the
 >     resulting information as random FPA register contents;
 >     this could perhaps be avoided by always dumping a dummy
 >     FPA regs structure at the start of this note, though this
 >     seems wasteful.
 > 
 >     Possibly, gdb may explode if the NT_FPREGSET note is larger
 >     then expected.  I've not investigated this.
 > 
 >  b) Dump the VFP/NEON regs in a note of type NT_PRXFPREG.
 >     This avoids defining a new note type; however, this note type
 >     is really for x86 -- so overloading it in this way may just
 >     be nonsensical / confusing.
 > 
 >  c) Create a new note type specially for this, and dump the
 >     VFP/NEON regs in there.  This is closest to the model followed
 >     by ptrace, where PTRACE_GETREGS, PTRACE_GETFPREGS and
 >     PTRAGE_GETVFPREGS are all distinct.
 > 
 > 
 > Going for (c) seems sanest to me, and also means that old gdb
 > will just ignore the new data; this is probably what we want.
 > 
 > Comments are welcome.

Strongly prefer (c) or (b).  (a) should be avoided if at all
possible: appending to or changing established types is just
asking for trouble, unless those types were designed from the
start to be self-describing and extensible.

RFC: ARM: Support for VFP/NEON registers in coredumps

[Ulrich Weigand]

Dave Martin <dave.martin@linaro.org> wrote:

>  c) Create a new note type specially for this, and dump the
>     VFP/NEON regs in there.  This is closest to the model followed
>     by ptrace, where PTRACE_GETREGS, PTRACE_GETFPREGS and
>     PTRAGE_GETVFPREGS are all distinct.

RFC: ARM: Support for VFP/NEON registers in coredumps

[Dave Martin]

On Thu, Mar 24, 2011 at 2:57 PM, Ulrich Weigand
<Ulrich.Weigand@de.ibm.com> wrote:
> Dave Martin <dave.martin@linaro.org> wrote:
>
>> ?c) Create a new note type specially for this, and dump the
>> ? ? VFP/NEON regs in there. ?This is closest to the model followed
>> ? ? by ptrace, where PTRACE_GETREGS, PTRACE_GETFPREGS and
>> ? ? PTRAGE_GETVFPREGS are all distinct.
>
> From a GDB perspective, I'd consider this the preferred solution;
> there are precedents for this on other platforms (e.g. PowerPC),
> and there is existing GDB/BFD infrastructure to check for presence/
> absence of such notes.
>
> However, we also need to be able to distinguish the various cases:
> VFPv2 vs. VFPv3-D16 vs. VFPv3-D32 vs. VFPv32-D32+NEON
> These cause GDB to display/interact with the register set in
> different ways ...
>
> To distinguish 16 vs. 32 registers, we can of course simply look
> at the size of the note. ?But how to detect support for NEON as
> opposed to plain VFPv32-D32? ?I could think of two ways:
>
> 1) Have *two* new note types, one for VFP, and one for NEON

That may not make sense, since really it's a single register file
shared by VFP and NEON.  With NEON, you always have 32 x 64-bit regs,
but it's possible (although rare) to have this many regs on ARMv7 even
if NEON is absent.

> 2) Have GDB look into the AT_HWCAP setting in the NT_AUXV note
>
> Option 2) seems preferable to me, since NT_AUXV is already there,
> and it can also be used to detect the integer-only NEON case.

We could also dump the relevant hardware capability registers, which
can be more informative, though I'm in two minds about whether this is
appropriate / beneficial.  A layout something like this might work:

NT_VFPREGSET*

    unsigned long flags;
    unsigned long feature_registers[3];
    unsigned long fpscr;
    unsigned long long regs[16 or 32];

There are currently 3 relevant feature registers, the main
floating-point ID register FPSID, and the floating-point / SIMD
feature registers MVFR0, MVFR1.

In either case, we define the contents of the flags field in such a
way as to allow gdb to understand the format, and to allow for future
expansion if this is ever needed.

(*The note types seem to use different names in linux/elf.h compared
with /usr/include/elf.h and GDB.  I've followed the outside world's
convention here.)

Does this look workable?

---Dave

>
> Thoughts?
>
>
> Mit freundlichen Gruessen / Best Regards
>
> Ulrich Weigand
>
> --
> ?Dr. Ulrich Weigand | Phone: +49-7031/16-3727
> ?STSM, GNU compiler and toolchain for Linux on System z and Cell/B.E.
> ?IBM Deutschland Research & Development GmbH
> ?Vorsitzender des Aufsichtsrats: Martin Jetter | Gesch?ftsf?hrung: Dirk
> Wittkopp
> ?Sitz der Gesellschaft: B?blingen | Registergericht: Amtsgericht
> Stuttgart, HRB 243294
>
>

RFC: ARM: Support for VFP/NEON registers in coredumps

[Ulrich Weigand]

Dave Martin <dave.martin@linaro.org> wrote:
> On Thu, Mar 24, 2011 at 2:57 PM, Ulrich Weigand
<Ulrich.Weigand@de.ibm.com> wrote:
> > 1) Have *two* new note types, one for VFP, and one for NEON
>
> That may not make sense, since really it's a single register file
> shared by VFP and NEON.  With NEON, you always have 32 x 64-bit regs,
> but it's possible (although rare) to have this many regs on ARMv7 even
> if NEON is absent.

OK, agreed.

> > 2) Have GDB look into the AT_HWCAP setting in the NT_AUXV note
> >
> > Option 2) seems preferable to me, since NT_AUXV is already there,
> > and it can also be used to detect the integer-only NEON case.
>
> We could also dump the relevant hardware capability registers, which
> can be more informative, though I'm in two minds about whether this is
> appropriate / beneficial.  A layout something like this might work:
>
> NT_VFPREGSET*
>
>     unsigned long flags;
>     unsigned long feature_registers[3];
>     unsigned long fpscr;
>     unsigned long long regs[16 or 32];
>
> There are currently 3 relevant feature registers, the main
> floating-point ID register FPSID, and the floating-point / SIMD
> feature registers MVFR0, MVFR1.
>
> In either case, we define the contents of the flags field in such a
> way as to allow gdb to understand the format, and to allow for future
> expansion if this is ever needed.
>
> (*The note types seem to use different names in linux/elf.h compared
> with /usr/include/elf.h and GDB.  I've followed the outside world's
> convention here.)

I would prefer to keep the contents of NT_VFPREGSET identical to the
contents of the PTRACE_GETVPFREGS/PTRACE_SETVPFREGS buffer:

- This would allow implementation via the new generic user_regset_view
  mechanism (which the ARM kernel doesn't use yet, but probably should)

- In any case, GDB likes to have the same set of information available
  when debugging core files and native targets

So if there is indeed additional information that would be interesting,
I'd argue this should be presented as a *new* core file note, *and* as
a new set of PTRACE_... APIs.   (But for GDB's purpose NT_AUXV right
now is sufficent.  Everything else could be displayed to the user if
of interest.)


Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

--
  Dr. Ulrich Weigand | Phone: +49-7031/16-3727
  STSM, GNU compiler and toolchain for Linux on System z and Cell/B.E.
  IBM Deutschland Research & Development GmbH
  Vorsitzender des Aufsichtsrats: Martin Jetter | Gesch?ftsf?hrung: Dirk
Wittkopp
  Sitz der Gesellschaft: B?blingen | Registergericht: Amtsgericht
Stuttgart, HRB 243294

RFC: ARM: Support for VFP/NEON registers in coredumps

[Dave Martin]

On Thu, Mar 24, 2011 at 4:40 PM, Ulrich Weigand
<Ulrich.Weigand@de.ibm.com> wrote:
> Dave Martin <dave.martin@linaro.org> wrote:
>> On Thu, Mar 24, 2011 at 2:57 PM, Ulrich Weigand
> <Ulrich.Weigand@de.ibm.com> wrote:
>> > 1) Have *two* new note types, one for VFP, and one for NEON
>>
>> That may not make sense, since really it's a single register file
>> shared by VFP and NEON. ?With NEON, you always have 32 x 64-bit regs,
>> but it's possible (although rare) to have this many regs on ARMv7 even
>> if NEON is absent.
>
> OK, agreed.
>
>> > 2) Have GDB look into the AT_HWCAP setting in the NT_AUXV note
>> >
>> > Option 2) seems preferable to me, since NT_AUXV is already there,
>> > and it can also be used to detect the integer-only NEON case.
>>
>> We could also dump the relevant hardware capability registers, which
>> can be more informative, though I'm in two minds about whether this is
>> appropriate / beneficial. ?A layout something like this might work:
>>
>> NT_VFPREGSET*
>>
>> ? ? unsigned long flags;
>> ? ? unsigned long feature_registers[3];
>> ? ? unsigned long fpscr;
>> ? ? unsigned long long regs[16 or 32];
>>
>> There are currently 3 relevant feature registers, the main
>> floating-point ID register FPSID, and the floating-point / SIMD
>> feature registers MVFR0, MVFR1.
>>
>> In either case, we define the contents of the flags field in such a
>> way as to allow gdb to understand the format, and to allow for future
>> expansion if this is ever needed.
>>
>> (*The note types seem to use different names in linux/elf.h compared
>> with /usr/include/elf.h and GDB. ?I've followed the outside world's
>> convention here.)
>
> I would prefer to keep the contents of NT_VFPREGSET identical to the
> contents of the PTRACE_GETVPFREGS/PTRACE_SETVPFREGS buffer:
>
> - This would allow implementation via the new generic user_regset_view
> ?mechanism (which the ARM kernel doesn't use yet, but probably should)
>
> - In any case, GDB likes to have the same set of information available
> ?when debugging core files and native targets
>
> So if there is indeed additional information that would be interesting,
> I'd argue this should be presented as a *new* core file note, *and* as
> a new set of PTRACE_... APIs. ? (But for GDB's purpose NT_AUXV right
> now is sufficent. ?Everything else could be displayed to the user if
> of interest.)

OK, that sounds reasonable.

If I'm reading the kernel code correctly, the size of the data
returned by PTRACE_GETVFPREGS is already dependent on the number of
registers (D16/D32) implemented by the hardware, and this is
determinable from the auxv contents; so I guess we don't have a new
problem to solve here.

The other info can always be added in a later pass, if it looks like
it would be useful...

Cheers
---Dave

RFC: ARM: Support for VFP/NEON registers in coredumps

[Ulrich Weigand]

Dave Martin <dave.martin@linaro.org> wrote:
> On Thu, Mar 24, 2011 at 4:40 PM, Ulrich Weigand
<Ulrich.Weigand@de.ibm.com> wrote:
> > I would prefer to keep the contents of NT_VFPREGSET identical to the
> > contents of the PTRACE_GETVPFREGS/PTRACE_SETVPFREGS buffer:
> >
> > - This would allow implementation via the new generic user_regset_view
> > ?mechanism (which the ARM kernel doesn't use yet, but probably should)
> >
> > - In any case, GDB likes to have the same set of information available
> > ?when debugging core files and native targets
> >
> > So if there is indeed additional information that would be interesting,
> > I'd argue this should be presented as a *new* core file note, *and* as
> > a new set of PTRACE_... APIs. ? (But for GDB's purpose NT_AUXV right
> > now is sufficent. ?Everything else could be displayed to the user if
> > of interest.)
>
> OK, that sounds reasonable.
>
> If I'm reading the kernel code correctly, the size of the data
> returned by PTRACE_GETVFPREGS is already dependent on the number of
> registers (D16/D32) implemented by the hardware, and this is
> determinable from the auxv contents; so I guess we don't have a new
> problem to solve here.
>
> The other info can always be added in a later pass, if it looks like
> it would be useful...

OK, agreed.

So to summarize: the kernel will write additional note sections as if
generated via user_regset_view, containing the PTRACE_GETVFPREGS data.

Note name: "LINUX"
Note type: t.b.d. [*]

[*] Looking at elf.h a logical name/value might be:

#define NT_ARM_VFP            0x400		/* ARM VFP/NEON registers */


GDB support along those lines ought to be straightforward.



Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

--
  Dr. Ulrich Weigand | Phone: +49-7031/16-3727
  STSM, GNU compiler and toolchain for Linux on System z and Cell/B.E.
  IBM Deutschland Research & Development GmbH
  Vorsitzender des Aufsichtsrats: Martin Jetter | Gesch?ftsf?hrung: Dirk
Wittkopp
  Sitz der Gesellschaft: B?blingen | Registergericht: Amtsgericht
Stuttgart, HRB 243294

RFC: ARM: Support for VFP/NEON registers in coredumps

[Dave Martin]

On Thu, Mar 24, 2011 at 6:11 PM, Ulrich Weigand
<Ulrich.Weigand@de.ibm.com> wrote:
> Dave Martin <dave.martin@linaro.org> wrote:
>> On Thu, Mar 24, 2011 at 4:40 PM, Ulrich Weigand
> <Ulrich.Weigand@de.ibm.com> wrote:
>> > I would prefer to keep the contents of NT_VFPREGSET identical to the
>> > contents of the PTRACE_GETVPFREGS/PTRACE_SETVPFREGS buffer:
>> >
>> > - This would allow implementation via the new generic user_regset_view
>> > ?mechanism (which the ARM kernel doesn't use yet, but probably should)
>> >
>> > - In any case, GDB likes to have the same set of information available
>> > ?when debugging core files and native targets
>> >
>> > So if there is indeed additional information that would be interesting,
>> > I'd argue this should be presented as a *new* core file note, *and* as
>> > a new set of PTRACE_... APIs. ? (But for GDB's purpose NT_AUXV right
>> > now is sufficent. ?Everything else could be displayed to the user if
>> > of interest.)
>>
>> OK, that sounds reasonable.
>>
>> If I'm reading the kernel code correctly, the size of the data
>> returned by PTRACE_GETVFPREGS is already dependent on the number of
>> registers (D16/D32) implemented by the hardware, and this is
>> determinable from the auxv contents; so I guess we don't have a new
>> problem to solve here.
>>
>> The other info can always be added in a later pass, if it looks like
>> it would be useful...
>
> OK, agreed.
>
> So to summarize: the kernel will write additional note sections as if
> generated via user_regset_view, containing the PTRACE_GETVFPREGS data.
>
> Note name: "LINUX"

Why "LINUX" and not "CORE"?  I don't understand the distinction... are
the "CORE" notes common to all platforms / all ELF implementations?

> Note type: t.b.d. [*]
>
> [*] Looking at elf.h a logical name/value might be:
>
> #define NT_ARM_VFP ? ? ? ? ? ?0x400 ? ? ? ? ? ? /* ARM VFP/NEON registers */
>
>
> GDB support along those lines ought to be straightforward.

It's been suggested that the new note should include a version/flags
field alongside the ptrace-like register dump, so that if the format
turns out to be inadequate / broken, it can be extended in a
compatible way.

However, nothing else in the coredump or the ptrace interface seems to
have such versioning implementation.  Ptrace gets extended by adding
more and more ptrace call types instead.  Adding version fields, while
sensible, seems inconsistent with the current implementation.

What's your view on adding a flags field to the VFP state dump?

Cheers
---Dave

RFC: ARM: Support for VFP/NEON registers in coredumps

[Ulrich Weigand]

Dave Martin <dave.martin@linaro.org> wrote:
> > So to summarize: the kernel will write additional note sections as if
> > generated via user_regset_view, containing the PTRACE_GETVFPREGS data.
> >
> > Note name: "LINUX"
>
> Why "LINUX" and not "CORE"?  I don't understand the distinction... are
> the "CORE" notes common to all platforms / all ELF implementations?

Because that's what user_regset_view would do; all notes except the
standard ones (NT_PRPSINFO, NT_PRSTATUS, NT_PRFPREG, NT_AUXV) have a
note name of "LINUX".   I'm not completely sure about the rationale,
but presumably it is indeed because the standard notes are more or less
common across multiple platforms.

> > Note type: t.b.d. [*]
> >
> > [*] Looking at elf.h a logical name/value might be:
> >
> > #define NT_ARM_VFP ? ? ? ? ? ?0x400 ? ? ? ? ? ? /* ARM VFP/NEON
registers */
> >
> >
> > GDB support along those lines ought to be straightforward.
>
> It's been suggested that the new note should include a version/flags
> field alongside the ptrace-like register dump, so that if the format
> turns out to be inadequate / broken, it can be extended in a
> compatible way.
>
> However, nothing else in the coredump or the ptrace interface seems to
> have such versioning implementation.  Ptrace gets extended by adding
> more and more ptrace call types instead.  Adding version fields, while
> sensible, seems inconsistent with the current implementation.
>
> What's your view on adding a flags field to the VFP state dump?

Again, if we want to use (or mimic) the user_regset_view mechanism,
there is no choice in any of this; the content of the note will be
exactly identical to the content of the ptrace buffer.  Since this
is the way everybody else is using, I think we'd have to have really
good arguments for deviating from it; I'm not sure I see those.

The usual way to deal with changes to the register set is to define
*new* regset structures, which then translates to new ptrace commands
and new core files notes, which are used instead of or in addition to
the old ones ...


Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

--
  Dr. Ulrich Weigand | Phone: +49-7031/16-3727
  STSM, GNU compiler and toolchain for Linux on System z and Cell/B.E.
  IBM Deutschland Research & Development GmbH
  Vorsitzender des Aufsichtsrats: Martin Jetter | Gesch?ftsf?hrung: Dirk
Wittkopp
  Sitz der Gesellschaft: B?blingen | Registergericht: Amtsgericht
Stuttgart, HRB 243294

RFC: ARM: Support for VFP/NEON registers in coredumps

[Dave Martin]

On Fri, Mar 25, 2011 at 1:11 PM, Ulrich Weigand
<Ulrich.Weigand@de.ibm.com> wrote:
> Dave Martin <dave.martin@linaro.org> wrote:
>> > So to summarize: the kernel will write additional note sections as if
>> > generated via user_regset_view, containing the PTRACE_GETVFPREGS data.
>> >
>> > Note name: "LINUX"
>>
>> Why "LINUX" and not "CORE"? ?I don't understand the distinction... are
>> the "CORE" notes common to all platforms / all ELF implementations?
>
> Because that's what user_regset_view would do; all notes except the
> standard ones (NT_PRPSINFO, NT_PRSTATUS, NT_PRFPREG, NT_AUXV) have a
> note name of "LINUX". ? I'm not completely sure about the rationale,
> but presumably it is indeed because the standard notes are more or less
> common across multiple platforms.

OK -- I'm happy to go along with this.  It's easy to change if someone
doesn't like it, though I expect it will be accepted as-is.

>
>> > Note type: t.b.d. [*]
>> >
>> > [*] Looking at elf.h a logical name/value might be:
>> >
>> > #define NT_ARM_VFP ? ? ? ? ? ?0x400 ? ? ? ? ? ? /* ARM VFP/NEON
> registers */
>> >
>> >
>> > GDB support along those lines ought to be straightforward.
>>
>> It's been suggested that the new note should include a version/flags
>> field alongside the ptrace-like register dump, so that if the format
>> turns out to be inadequate / broken, it can be extended in a
>> compatible way.
>>
>> However, nothing else in the coredump or the ptrace interface seems to
>> have such versioning implementation. ?Ptrace gets extended by adding
>> more and more ptrace call types instead. ?Adding version fields, while
>> sensible, seems inconsistent with the current implementation.
>>
>> What's your view on adding a flags field to the VFP state dump?
>
> Again, if we want to use (or mimic) the user_regset_view mechanism,
> there is no choice in any of this; the content of the note will be
> exactly identical to the content of the ptrace buffer. ?Since this
> is the way everybody else is using, I think we'd have to have really
> good arguments for deviating from it; I'm not sure I see those.
>
> The usual way to deal with changes to the register set is to define
> *new* regset structures, which then translates to new ptrace commands
> and new core files notes, which are used instead of or in addition to
> the old ones ...

I was indeed thinking that it might be a good idea to take this
opportunity to migrate to using regsets; though for simplicity, I'll
probably avoid this for the first iteration.

Will this have any impact on the ptrace interface?

Cheers
---Dave

RFC: ARM: Support for VFP/NEON registers in coredumps

[Ulrich Weigand]

Dave Martin <dave.martin@linaro.org> wrote:

> I was indeed thinking that it might be a good idea to take this
> opportunity to migrate to using regsets; though for simplicity, I'll
> probably avoid this for the first iteration.
>
> Will this have any impact on the ptrace interface?

Well, once you support regsets, the PTRACE_GETREGSET/PTRACE_SETREGSET
commands will start working.  However, on platforms that already had
commands to access registers, platform-specific code will of course
need to continue to support those.

See e.g. arch/powerpc/kernel/ptrace.c, which by now implements all
of its legacy ptrace register commands in terms of regset functions.


Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

--
  Dr. Ulrich Weigand | Phone: +49-7031/16-3727
  STSM, GNU compiler and toolchain for Linux on System z and Cell/B.E.
  IBM Deutschland Research & Development GmbH
  Vorsitzender des Aufsichtsrats: Martin Jetter | Gesch?ftsf?hrung: Dirk
Wittkopp
  Sitz der Gesellschaft: B?blingen | Registergericht: Amtsgericht
Stuttgart, HRB 243294

RFC: ARM: Support for VFP/NEON registers in coredumps

[Dave Martin]

On Fri, Mar 25, 2011 at 1:40 PM, Ulrich Weigand
<Ulrich.Weigand@de.ibm.com> wrote:
> Dave Martin <dave.martin@linaro.org> wrote:
>
>> I was indeed thinking that it might be a good idea to take this
>> opportunity to migrate to using regsets; though for simplicity, I'll
>> probably avoid this for the first iteration.
>>
>> Will this have any impact on the ptrace interface?
>
> Well, once you support regsets, the PTRACE_GETREGSET/PTRACE_SETREGSET
> commands will start working. ?However, on platforms that already had
> commands to access registers, platform-specific code will of course
> need to continue to support those.

Ah, right -- I wondered if there was something like this, but
initially I'd just been looking at the ptrace man page; I guess it's
rather too out-of-date for this.

>
> See e.g. arch/powerpc/kernel/ptrace.c, which by now implements all
> of its legacy ptrace register commands in terms of regset functions.

OK, I'll take a look -- thanks.

---Dave