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 3:29 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 15:03:48 +0100, Metzger, Markus T wrote:
> > > There is i386-nat.c for the common functions between these two files.
> >
> > Is it OK put Linux specific code into i386-nat.c?
> 
> True it is not so clear, it would be OK as long as the linux_supports_btrace()
> call is moved out of it, as otherwise it just checks the CPU hardware feature.
> 
> But as you use it also in gdbserver I see now it can be moved to
> common/linux-btrace.[ch] with appropriate #ifdef __i386__ and __x86_64__.
> common/ currently does not have any per-file arch/target configury like gdb/
> and gdbserver/ have, one day it will probably have it but not now.

I can do this. It should also simplify some of the code if I can do the check there.

Can I expect that others will be OK with this, as well?


> > 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.
> 
> /proc/cpuinfo does contain this information:
> 	cpu family  : 6
> 	model	: 42
> 
> I preferred /proc/cpuinfo, for example in virtualization environments with
> buggy emulations I find easier to fake /proc/cpuinfo than the cpuid
> instruction data.
> 
> I find gdb/go32-nat.c and gdb/gdbserver/linux-tic6x-low.c not so relevant key
> as it has marginal market share compared to x86*.
> 
> But I am OK with the cpuid instruction when it is already in GDB.
> 
> 
> > 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.
> 
> Not speaking for Mark but I also would like such check in Linux kernel
> otherwise Linux kernel provides via SYS_perf_event_open invalid data.

Agreed.


> With Linux kernel trunk having such fix we can talk how to workaround older
> kernels, I agree with you a workaround for older kernels should be there as
> you have implemented.  But such workaround could be limited somehow, for
> example either checking /proc/version whether we run on a buggy kernel or for
> example enabling the feature on any CPU model >= 90 as according to Intel
> numbering all those CPUs should have the feature either missing or correct
> (but never buggy).
> 
> Maintaining the list of CPUs twice in both GDB and Linux kernel does not seem
> great to me.

To me neither.

Checking the kernel version sounds good. We would only need to do the cpuid check for older kernels. I could add this once the kernel patch has been accepted and merged. Is there already some code in gdb that parses the output of /proc/version?

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