This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RE: [patch v4 13/13] btrace, x86: restrict to Atom
> -----Original Message-----
> From: Jan Kratochvil [mailto:jan.kratochvil@redhat.com]
> Sent: Tuesday, November 27, 2012 2:05 PM
> To: Metzger, Markus T
> Cc: gdb-patches@sourceware.org; markus.t.metzger@gmail.com; palves@redhat.com; tromey@redhat.com; kettenis@gnu.org
> Subject: Re: [patch v4 13/13] btrace, x86: restrict to Atom
>
> On Tue, 27 Nov 2012 11:49:11 +0100, markus.t.metzger@intel.com wrote:
> > 2012-11-27 Markus Metzger <markus.t.metzger@intel.com>
> >
> > * amd64-linux-nat.c (amd64_linux_supports_btrace): New.
> > (_initialize_amd64_linux_nat): Change supports_btrace method.
> > * i386-linux-nat.c (i386_linux_supports_btrace): New.
> > (_initialize_i386_linux_nat): Change supports_btrace method.
>
> There is i386-nat.c for the common functions between these two files.
Is it OK put Linux specific code into i386-nat.c? The cpuid check would be generic IA but the btrace feature is Linux-only.
> > --- a/gdb/gdbserver/linux-x86-low.c
> > +++ b/gdb/gdbserver/linux-x86-low.c
> > @@ -1859,6 +1859,36 @@ x86_get_min_fast_tracepoint_insn_len (void)
> > }
> > }
> >
> > +/* See linux-low.h. */
> > +static int
> > +x86_supports_btrace (void)
> > +{
> > + unsigned int cpuid, model, family;
> > +
> > + __asm__ __volatile__ ("movl $1, %%eax;"
> > + "cpuid;"
> > + : "=a" (cpuid)
> > + :: "%ebx", "%ecx", "%edx");
>
> As Mark Kettenis expressed issue with '__asm__ __volatile__' here and I also
> have some uncertainty about its compatibility (*), as it is in linux-* file
> anyway I wound find more safe and more common style in GDB to just read and
> parse "/proc/cpuinfo",
>
> (*) Although probably any Linux compiler needst so support it the GCC
> compatible way, incl. llvm and icc.
I took the __asm__ __volatile__ code from gdb/go32-nat.c. There's similar inline assembly code in gdb/gdbserver/linux-tic6x-low.c for checking the cpu id on that architecture.
The go32 code is also checking for features. I'm not sure whether this can be done by parsing /proc/cpuinfo.
I think Marks point is that he does not want any such check in gdb but rather have the kernel handle it. He's right that the kernel should handle it. I just think that gdb needs to handle it, as well.
> > +
> > + family = (cpuid >> 8) & 0xf;
> > + model = (cpuid >> 4) & 0xf;
> > +
> > + switch (family)
> > + {
> > + case 6:
> > + model += (cpuid >> 12) & 0xf0;
> > +
> > + switch (model)
> > + {
>
> Here should be tab, not 8 spaces.
Thanks. Fixed. I had this in all three functions.
> > + case 28: /* Atom. */
> > + case 38:
>
> In some instances of this function you list 39 and in other instances you do
> not.
Thanks. I added 39.
Regards,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Peter Gleissner, Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052