[PATCH v2 08/13] btrace: move and rename btrace-common

[Metzger, Markus T]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Pedro Alves
> Sent: Tuesday, January 27, 2015 12:17 PM
> To: Metzger, Markus T
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH v2 08/13] btrace: move and rename btrace-common
> 
> On 11/20/2014 10:47 AM, Markus Metzger wrote:
> > diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> > index ebacd96..4ed9837 100644
> > --- a/gdb/gdbserver/server.c
> > +++ b/gdb/gdbserver/server.c
> > @@ -30,7 +30,7 @@
> >  #endif
> >  #include "gdb_vecs.h"
> >  #include "gdb_wait.h"
> > -#include "btrace-common.h"
> > +#include "nat/x86-btrace.h"
> >  #include "filestuff.h"
> >  #include "tracepoint.h"
> >  #include "dll.h"
> 
> This still looks like layering violation this way.
> server.c should not really have x86 specific bits either.
> 
> My original comment on v1 was:
> 
> >> diff --git a/gdb/common/btrace-common.c b/gdb/common/btrace-
> common.c
> >> > index 90774a2..178ad35 100644
> >> > --- a/gdb/common/btrace-common.c
> >> > +++ b/gdb/common/btrace-common.c
> >> > @@ -18,10 +18,47 @@
> >> >     along with this program.  If not, see <http://www.gnu.org/licenses/>.
> */
> >> >
> >> >  #include "btrace-common.h"
> >> > +#include "nat/i386-cpuid.h"
> > Hm, this is a layering violation.
> > common/ files must not include nat/ files.
> > If btrace-common.c is only used by native code, then it should
> > itself be in nat/ too.
> 
> So, what does btrace-common.h actually contain that server.c needs?
> "common" as a moniker doesn't really help much, as it doesn't
> really indicate what the code is about.  Is it just common
> definitions that target-independent parts of gdb and/or gdbserver
> use?  How about something like leaving those parts in common/ (bonus
> points of naming the file something more indicative or what is
> contains), and then the native bits, like the x86 probing would
> be put under nat/x86-btrace.h|c.

What introduced the x86 dependency was the new function
btrace_this_cpu, which needs x86-cpuid.h.

I put the declaration of struct btrace_cpu in btrace-common.h
because it will be needed for struct btrace_data_pt_config, which will
be needed by struct btrace_data.

The new function btrace_this_cpu goes into nat/linux-btrace.c where
it is used.  Now nat/linux-btrace.c has an x86 dependency.  Otoh, the
entire feature is x86-dependent so we might as well rename it to
nat/x86-linux-btrace.c, if you want.

I dropped this patch and merged the two cpu identification patches.

Regarding a better name, the file contains configuration and data
representation stuff.  It could be split into btrace-config.h and
btrace-data.h.  Or we could just leave it as btrace-common.h.

regards,
markus.

Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: 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