RE: [patch v4 13/13] btrace, x86: restrict to Atom

["Metzger, Markus T" <markus.t.metzger@intel.com>]

> -----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