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

[Pedro Alves <palves at redhat dot com>]

On 11/27/2012 03:13 PM, Metzger, Markus T wrote:
>> -----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?

Well, I don't agree with _that_ reasoning.  We don't have _any_ configury in common/,
because common/ is not a library.  The configury is in gdb/ and gdb/gdbserver/.  I don't
see any issue preventing splitting architecture specific code to separate files, as we do
in gdb/ and gdbserver/.  It's just that the source file is named common/foo.c rather than
foo.c. That said, btrace is inherently x86-specific, right?  Is there any other
architecture that does something of the sort?  If not, then I agree with
assuming x86 in the file, and #ifdef where necessary to distinguish 32-bit/64-bit.

-- 
Pedro Alves