Re: [Patch] libgcov.c re-factoring
On Mon, Jan 6, 2014 at 9:49 AM, Teresa Johnson tejohn...@google.com wrote:
On Sun, Jan 5, 2014 at 12:08 PM, Jan Hubicka hubi...@ucw.cz wrote:
2014-01-03 Rong Xu x...@google.com
* gcc/gcov-io.c (gcov_var): Move from gcov-io.h.
(gcov_position): Ditto.
(gcov_is_error): Ditto.
(gcov_rewrite): Ditto.
* gcc/gcov-io.h: Refactor. Move gcov_var to gcov-io.h, and libgcov
only part to libgcc/libgcov.h.
* libgcc/libgcov-driver.c: Use libgcov.h.
(buffer_fn_data): Use xmalloc instead of malloc.
(gcov_exit_merge_gcda): Ditto.
* libgcc/libgcov-driver-system.c (allocate_filename_struct): Ditto.
* libgcc/libgcov.h: New common header files for libgcov-*.h.
* libgcc/libgcov-interface.c: Use libgcov.h
* libgcc/libgcov-merge.c: Ditto.
* libgcc/libgcov-profiler.c: Ditto.
* libgcc/Makefile.in: Add dependence to libgcov.h
OK, with the licence changes and...
Index: gcc/gcov-io.c
===
--- gcc/gcov-io.c (revision 206100)
+++ gcc/gcov-io.c (working copy)
@@ -36,6 +36,61 @@ static const gcov_unsigned_t *gcov_read_words (uns
static void gcov_allocate (unsigned);
#endif
+/* Optimum number of gcov_unsigned_t's read from or written to disk. */
+#define GCOV_BLOCK_SIZE (1 10)
+
+GCOV_LINKAGE struct gcov_var
+{
+ FILE *file;
+ gcov_position_t start; /* Position of first byte of block */
+ unsigned offset; /* Read/write position within the block. */
+ unsigned length; /* Read limit in the block. */
+ unsigned overread; /* Number of words overread. */
+ int error; /* 0 overflow, 0 disk error. */
+ int mode;/* 0 writing, 0 reading */
+#if IN_LIBGCOV
+ /* Holds one block plus 4 bytes, thus all coverage reads writes
+ fit within this buffer and we always can transfer GCOV_BLOCK_SIZE
+ to and from the disk. libgcov never backtracks and only writes 4
+ or 8 byte objects. */
+ gcov_unsigned_t buffer[GCOV_BLOCK_SIZE + 1];
+#else
+ int endian; /* Swap endianness. */
+ /* Holds a variable length block, as the compiler can write
+ strings and needs to backtrack. */
+ size_t alloc;
+ gcov_unsigned_t *buffer;
+#endif
+} gcov_var;
+
+/* Save the current position in the gcov file. */
+static inline gcov_position_t
+gcov_position (void)
+{
+ gcc_assert (gcov_var.mode 0);
+ return gcov_var.start + gcov_var.offset;
+}
+
+/* Return nonzero if the error flag is set. */
+static inline int
+gcov_is_error (void)
+{
+ return gcov_var.file ? gcov_var.error : 1;
+}
+
+#if IN_LIBGCOV
+/* Move to beginning of file and initialize for writing. */
+GCOV_LINKAGE inline void
+gcov_rewrite (void)
+{
+ gcc_assert (gcov_var.mode 0);
I would turn those two asserts into checking asserts so they do not
bloat the runtime lib.
Ok, but note that there are a number of other gcc_assert already in
gcov-io.c (these were the only 2 in gcov-io.h, now moved here). Should
I go ahead and change all of them in gcov-io.c?
Actually, I tried changing these two, but gcc_checking_assert is
undefined in libgcov.a. Ok to commit without this change?
Teresa
Thanks,
Teresa
Thanks,
Honza
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
Re: [Patch] libgcov.c re-factoring
Actually, I tried changing these two, but gcc_checking_assert is undefined in libgcov.a. Ok to commit without this change? OK. incrementally can you please define gcov_nonruntime_assert that will wind into gcc_assert for code within gcc/coverage tools and into nothing for libgcov runtime and we can change those offenders to that. Honza Teresa Thanks, Teresa Thanks, Honza -- Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413 -- Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
Re: [Patch] libgcov.c re-factoring
On Wed, Jan 8, 2014 at 6:34 AM, Jan Hubicka hubi...@ucw.cz wrote: Actually, I tried changing these two, but gcc_checking_assert is undefined in libgcov.a. Ok to commit without this change? OK. incrementally can you please define gcov_nonruntime_assert that will wind into gcc_assert for code within gcc/coverage tools and into nothing for libgcov runtime and we can change those offenders to that. Ok, committed as r206435. Will send the assert patch in a follow-up later this week. Teresa Honza Teresa Thanks, Teresa Thanks, Honza -- Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413 -- Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413 -- Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
Re: [Patch] libgcov.c re-factoring
On 01/07/2014 06:47 PM, Xinliang David Li wrote: A related question. I have not followed the header file restructuring discussion. Is there a documentation on header file structure and how they are organized? In a new .c file, simply including gimple.h would require many rounds of iterations to find missing headers via trial (compilation) and error. thanks, Not as yet... right now its in a cleanup phase where we are trying to flatten it out so we can get see where things are actually coming from and how they are used, and then relocate/reorganize things into sensible places. In an ideal world, Id like to eventually see the core component files, tree.h, gimple.h and rtl.h include all the basic files those components need, then each .c file can add whatever else is needed... but we'll see when we get closer. Andrew PS In reality, for now creating a new .c file probably isn't much different than it was before... you find some other .c file, copy the #includes from that, and go from there :-)
Re: [Patch] libgcov.c re-factoring
On Tue, Jan 7, 2014 at 7:42 PM, Andrew MacLeod amacl...@redhat.com wrote: On 01/07/2014 06:47 PM, Xinliang David Li wrote: A related question. I have not followed the header file restructuring discussion. Is there a documentation on header file structure and how they are organized? In a new .c file, simply including gimple.h would require many rounds of iterations to find missing headers via trial (compilation) and error. thanks, Not as yet... right now its in a cleanup phase where we are trying to flatten it out so we can get see where things are actually coming from and how they are used, and then relocate/reorganize things into sensible places. In an ideal world, Id like to eventually see the core component files, tree.h, gimple.h and rtl.h include all the basic files those components need, then each .c file can add whatever else is needed... but we'll see when we get closer. Yes -- wrapper headers can be introduced to form a hierarchy of headers. Each core areas in GCC deserves one such header. For instance, all interfaces for IR browsing, all interfaces for IR manipulation, IPA (cgraph, symtab, varpool etc), cfg, aliaser etc. Andrew PS In reality, for now creating a new .c file probably isn't much different than it was before... you find some other .c file, copy the #includes from that, and go from there :-) yeah. In the ideal world, we should be able to not rely on cutpaste and easily come up with the minimal set of headers needed ... thanks, David
Re: [Patch] libgcov.c re-factoring
On 12/22/2013 01:27 PM, Jan Hubicka wrote: I believe when the code was created by moving it from elsehwre, the copyright should say original date of gcov-io.h. + +#include tconfig.h +#include tsystem.h +#include coretypes.h +#include tm.h +#include libgcc_tm.h I would really like someone working on header restructuring to comment on those. I am not 100% sure what our best practices currently are in respect of including headers within headers and specially after good amount of defines like gcov-io.h gets included. Just back.. For the moment the goal with headers is to completely flatten them.. ie, we do not want to include header files from header files. Once there is a sensible restructuring of things in place, we are likely to reintroduce them in a controlled and sensible manner. So at the moment, I'd like to not include any additional .h files from within .h files... Andrew
Re: [Patch] libgcov.c re-factoring
On 12/22/2013 01:27 PM, Jan Hubicka wrote: I believe when the code was created by moving it from elsehwre, the copyright should say original date of gcov-io.h. + +#include tconfig.h +#include tsystem.h +#include coretypes.h +#include tm.h +#include libgcc_tm.h I would really like someone working on header restructuring to comment on those. I am not 100% sure what our best practices currently are in respect of including headers within headers and specially after good amount of defines like gcov-io.h gets included. Just back.. For the moment the goal with headers is to completely flatten them.. OK. Seems sensible. I will push IPA/cgraph/symtab in this direction next stage1. ie, we do not want to include header files from header files. Once there is a sensible restructuring of things in place, we are likely to reintroduce them in a controlled and sensible manner. So at the moment, I'd like to not include any additional .h files from within .h files... The situation here is slightly more slipperly than in other cases, since we have headers (gcov-io.h.h) that is shared across GCC coverage code and runtime (libgcov) and stand alone tools (gcov-tool and gcov). The header has several ifdefs that makes it to do the right thing in all 3 contexts that are different. The change in question is to pull out a lot of mess that is specific into runtime to libgcov specific header file that includes gcov-io.h later in game. In a way I see this as an improvement, since runtime bits are localized to place where rest of libgcov is (that is for some reason libgcc and not libgcov directory) rather than hidding it in geenral purpose header. I suppose it will also make easier to make libgcov portable to non-stdio based environments, like in kernel, that is google's desire. What would be preferred solution here? Honza Andrew
Re: [Patch] libgcov.c re-factoring
On 01/06/2014 09:37 AM, Jan Hubicka wrote: On 12/22/2013 01:27 PM, Jan Hubicka wrote: I believe when the code was created by moving it from elsehwre, the copyright should say original date of gcov-io.h. + +#include tconfig.h +#include tsystem.h +#include coretypes.h +#include tm.h +#include libgcc_tm.h I would really like someone working on header restructuring to comment on those. I am not 100% sure what our best practices currently are in respect of including headers within headers and specially after good amount of defines like gcov-io.h gets included. Just back.. For the moment the goal with headers is to completely flatten them.. OK. Seems sensible. I will push IPA/cgraph/symtab in this direction next stage1. ie, we do not want to include header files from header files. Once there is a sensible restructuring of things in place, we are likely to reintroduce them in a controlled and sensible manner. So at the moment, I'd like to not include any additional .h files from within .h files... The situation here is slightly more slipperly than in other cases, since we have headers (gcov-io.h.h) that is shared across GCC coverage code and runtime (libgcov) and stand alone tools (gcov-tool and gcov). The header has several ifdefs that makes it to do the right thing in all 3 contexts that are different. The change in question is to pull out a lot of mess that is specific into runtime to libgcov specific header file that includes gcov-io.h later in game. In a way I see this as an improvement, since runtime bits are localized to place where rest of libgcov is (that is for some reason libgcc and not libgcov directory) rather than hidding it in geenral purpose header. I suppose it will also make easier to make libgcov portable to non-stdio based environments, like in kernel, that is google's desire. What would be preferred solution here? Yeah, this isnt only a compiler header file, so what makes the most sense may be different. The rationale for flattening the compiler header files is to get a grasp on what is coming from where, regroup things in a more logical manner, then possibly reintroduce a more sensible include hierarchy. If this is improving the locality of the runtime then I wouldnt worry about it. we haven't gotten anywhere near that yet :-) I didnt realize on first glance that it was a libgcc file. if we ever do get that far, we can deal with it then :-) It isn't undoing anything we've done so far. Andrew
Re: [Patch] libgcov.c re-factoring
On Sun, Jan 5, 2014 at 12:08 PM, Jan Hubicka hubi...@ucw.cz wrote:
2014-01-03 Rong Xu x...@google.com
* gcc/gcov-io.c (gcov_var): Move from gcov-io.h.
(gcov_position): Ditto.
(gcov_is_error): Ditto.
(gcov_rewrite): Ditto.
* gcc/gcov-io.h: Refactor. Move gcov_var to gcov-io.h, and libgcov
only part to libgcc/libgcov.h.
* libgcc/libgcov-driver.c: Use libgcov.h.
(buffer_fn_data): Use xmalloc instead of malloc.
(gcov_exit_merge_gcda): Ditto.
* libgcc/libgcov-driver-system.c (allocate_filename_struct): Ditto.
* libgcc/libgcov.h: New common header files for libgcov-*.h.
* libgcc/libgcov-interface.c: Use libgcov.h
* libgcc/libgcov-merge.c: Ditto.
* libgcc/libgcov-profiler.c: Ditto.
* libgcc/Makefile.in: Add dependence to libgcov.h
OK, with the licence changes and...
Index: gcc/gcov-io.c
===
--- gcc/gcov-io.c (revision 206100)
+++ gcc/gcov-io.c (working copy)
@@ -36,6 +36,61 @@ static const gcov_unsigned_t *gcov_read_words (uns
static void gcov_allocate (unsigned);
#endif
+/* Optimum number of gcov_unsigned_t's read from or written to disk. */
+#define GCOV_BLOCK_SIZE (1 10)
+
+GCOV_LINKAGE struct gcov_var
+{
+ FILE *file;
+ gcov_position_t start; /* Position of first byte of block */
+ unsigned offset; /* Read/write position within the block. */
+ unsigned length; /* Read limit in the block. */
+ unsigned overread; /* Number of words overread. */
+ int error; /* 0 overflow, 0 disk error. */
+ int mode;/* 0 writing, 0 reading */
+#if IN_LIBGCOV
+ /* Holds one block plus 4 bytes, thus all coverage reads writes
+ fit within this buffer and we always can transfer GCOV_BLOCK_SIZE
+ to and from the disk. libgcov never backtracks and only writes 4
+ or 8 byte objects. */
+ gcov_unsigned_t buffer[GCOV_BLOCK_SIZE + 1];
+#else
+ int endian; /* Swap endianness. */
+ /* Holds a variable length block, as the compiler can write
+ strings and needs to backtrack. */
+ size_t alloc;
+ gcov_unsigned_t *buffer;
+#endif
+} gcov_var;
+
+/* Save the current position in the gcov file. */
+static inline gcov_position_t
+gcov_position (void)
+{
+ gcc_assert (gcov_var.mode 0);
+ return gcov_var.start + gcov_var.offset;
+}
+
+/* Return nonzero if the error flag is set. */
+static inline int
+gcov_is_error (void)
+{
+ return gcov_var.file ? gcov_var.error : 1;
+}
+
+#if IN_LIBGCOV
+/* Move to beginning of file and initialize for writing. */
+GCOV_LINKAGE inline void
+gcov_rewrite (void)
+{
+ gcc_assert (gcov_var.mode 0);
I would turn those two asserts into checking asserts so they do not
bloat the runtime lib.
Ok, but note that there are a number of other gcc_assert already in
gcov-io.c (these were the only 2 in gcov-io.h, now moved here). Should
I go ahead and change all of them in gcov-io.c?
Thanks,
Teresa
Thanks,
Honza
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
Re: [Patch] libgcov.c re-factoring
2014-01-03 Rong Xu x...@google.com
* gcc/gcov-io.c (gcov_var): Move from gcov-io.h.
(gcov_position): Ditto.
(gcov_is_error): Ditto.
(gcov_rewrite): Ditto.
* gcc/gcov-io.h: Refactor. Move gcov_var to gcov-io.h, and libgcov
only part to libgcc/libgcov.h.
* libgcc/libgcov-driver.c: Use libgcov.h.
(buffer_fn_data): Use xmalloc instead of malloc.
(gcov_exit_merge_gcda): Ditto.
* libgcc/libgcov-driver-system.c (allocate_filename_struct): Ditto.
* libgcc/libgcov.h: New common header files for libgcov-*.h.
* libgcc/libgcov-interface.c: Use libgcov.h
* libgcc/libgcov-merge.c: Ditto.
* libgcc/libgcov-profiler.c: Ditto.
* libgcc/Makefile.in: Add dependence to libgcov.h
OK, with the licence changes and...
Index: gcc/gcov-io.c
===
--- gcc/gcov-io.c (revision 206100)
+++ gcc/gcov-io.c (working copy)
@@ -36,6 +36,61 @@ static const gcov_unsigned_t *gcov_read_words (uns
static void gcov_allocate (unsigned);
#endif
+/* Optimum number of gcov_unsigned_t's read from or written to disk. */
+#define GCOV_BLOCK_SIZE (1 10)
+
+GCOV_LINKAGE struct gcov_var
+{
+ FILE *file;
+ gcov_position_t start; /* Position of first byte of block */
+ unsigned offset; /* Read/write position within the block. */
+ unsigned length; /* Read limit in the block. */
+ unsigned overread; /* Number of words overread. */
+ int error; /* 0 overflow, 0 disk error. */
+ int mode;/* 0 writing, 0 reading */
+#if IN_LIBGCOV
+ /* Holds one block plus 4 bytes, thus all coverage reads writes
+ fit within this buffer and we always can transfer GCOV_BLOCK_SIZE
+ to and from the disk. libgcov never backtracks and only writes 4
+ or 8 byte objects. */
+ gcov_unsigned_t buffer[GCOV_BLOCK_SIZE + 1];
+#else
+ int endian; /* Swap endianness. */
+ /* Holds a variable length block, as the compiler can write
+ strings and needs to backtrack. */
+ size_t alloc;
+ gcov_unsigned_t *buffer;
+#endif
+} gcov_var;
+
+/* Save the current position in the gcov file. */
+static inline gcov_position_t
+gcov_position (void)
+{
+ gcc_assert (gcov_var.mode 0);
+ return gcov_var.start + gcov_var.offset;
+}
+
+/* Return nonzero if the error flag is set. */
+static inline int
+gcov_is_error (void)
+{
+ return gcov_var.file ? gcov_var.error : 1;
+}
+
+#if IN_LIBGCOV
+/* Move to beginning of file and initialize for writing. */
+GCOV_LINKAGE inline void
+gcov_rewrite (void)
+{
+ gcc_assert (gcov_var.mode 0);
I would turn those two asserts into checking asserts so they do not
bloat the runtime lib.
Thanks,
Honza
Re: [Patch] libgcov.c re-factoring
On Sun, Dec 22, 2013 at 10:27 AM, Jan Hubicka hubi...@ucw.cz wrote:
On Tue, Dec 17, 2013 at 7:48 AM, Teresa Johnson tejohn...@google.com wrote:
On Mon, Dec 16, 2013 at 2:48 PM, Xinliang David Li davi...@google.com
wrote:
Ok -- gcov_write_counter and gcov_write_tag_length are qualified as
low level primitives for basic gcov format and probably should be kept
in gcov-io.c.
gcov_rewrite is petty much libgcov runtime implementation details so I
think it should be moved out. gcov_write_summary is not related to
gcov low level format either, neither is gcov_seek. Ok for them to be
moved?
After looking at these some more, with the idea that gcov-io.c should
encapsulate the lower level IO routines, then I think all of these
(including gcov_rewrite) should remain in gcov-io.c. I think
gcov_write_summary belongs there since all of the other gcov_write_*
Yep, I think gcov_write_summary and read summary should not be split in
between
two directories. Similary for gcov_seek/rewrite I can see either the whole
low-level I/O being abstracted away to -driver.c but currently -driver.c seem
to containing higher level stuff that you apparenlty want to fork for kernel
implementation instead and the actual i/o seems to remain in gcov-io.c
+GCOV_LINKAGE struct gcov_var gcov_var;
If gcov_var is not used by gcov-io.c only, why its declaration remains in
gcov-io.h?
Good point - moved to gcov-io.c.
Index: libgcc/libgcov.h
===
--- libgcc/libgcov.h(revision 0)
+++ libgcc/libgcov.h(revision 0)
@@ -0,0 +1,225 @@
+/* Header file for libgcov-*.c.
+ Contributed by Rong Xu x...@google.com.
+ Copyright (C) 2013 Free Software Foundation, Inc.
I believe when the code was created by moving it from elsehwre, the copyright
should say
original date of gcov-io.h.
Fixed the copyright header.
+
+#include tconfig.h
+#include tsystem.h
+#include coretypes.h
+#include tm.h
+#include libgcc_tm.h
I would really like someone working on header restructuring to comment on
those.
I am not 100% sure what our best practices currently are in respect of
including headers within headers and specially after good amount of
defines like gcov-io.h gets included.
Ok, I have left it the same as the prior patch for now. Note that
gcov-io.h was already including tconfig.h, which has now been moved to
libgcov.h. The other includes were pulled in from the various
libgcov-*.c files that were including them.
New patch (bootstrapped and regression tested) is included below.
+
+#include gcov-io.h
Otherwise the patch is OK (if header restructuring is fine and after
moving gcov_var structure definition into gcov-io.c if possible).
Ok, hopefully someone can comment on the header file issue.
Thanks,
Teresa
Honza
2014-01-03 Rong Xu x...@google.com
* gcc/gcov-io.c (gcov_var): Move from gcov-io.h.
(gcov_position): Ditto.
(gcov_is_error): Ditto.
(gcov_rewrite): Ditto.
* gcc/gcov-io.h: Refactor. Move gcov_var to gcov-io.h, and libgcov
only part to libgcc/libgcov.h.
* libgcc/libgcov-driver.c: Use libgcov.h.
(buffer_fn_data): Use xmalloc instead of malloc.
(gcov_exit_merge_gcda): Ditto.
* libgcc/libgcov-driver-system.c (allocate_filename_struct): Ditto.
* libgcc/libgcov.h: New common header files for libgcov-*.h.
* libgcc/libgcov-interface.c: Use libgcov.h
* libgcc/libgcov-merge.c: Ditto.
* libgcc/libgcov-profiler.c: Ditto.
* libgcc/Makefile.in: Add dependence to libgcov.h
Index: gcc/gcov-io.c
===
--- gcc/gcov-io.c (revision 206100)
+++ gcc/gcov-io.c (working copy)
@@ -36,6 +36,61 @@ static const gcov_unsigned_t *gcov_read_words (uns
static void gcov_allocate (unsigned);
#endif
+/* Optimum number of gcov_unsigned_t's read from or written to disk. */
+#define GCOV_BLOCK_SIZE (1 10)
+
+GCOV_LINKAGE struct gcov_var
+{
+ FILE *file;
+ gcov_position_t start; /* Position of first byte of block */
+ unsigned offset; /* Read/write position within the block. */
+ unsigned length; /* Read limit in the block. */
+ unsigned overread; /* Number of words overread. */
+ int error; /* 0 overflow, 0 disk error. */
+ int mode;/* 0 writing, 0 reading */
+#if IN_LIBGCOV
+ /* Holds one block plus 4 bytes, thus all coverage reads writes
+ fit within this buffer and we always can transfer GCOV_BLOCK_SIZE
+ to and from the disk. libgcov never backtracks and only writes 4
+ or 8 byte objects. */
+ gcov_unsigned_t buffer[GCOV_BLOCK_SIZE + 1];
+#else
+ int endian; /* Swap endianness. */
+ /* Holds a variable length block, as the compiler can write
+ strings and needs to backtrack. */
+ size_t alloc;
+
Re: [Patch] libgcov.c re-factoring
On Fri, 3 Jan 2014, Teresa Johnson wrote: Index: libgcc/libgcov.h === --- libgcc/libgcov.h(revision 0) +++ libgcc/libgcov.h(revision 0) @@ -0,0 +1,224 @@ +/* Header file for libgcov-*.c. + Copyright (C) 1996-2013 Free Software Foundation, Inc. + + This file is part of GCC. + + GCC is free software; you can redistribute it and/or modify it under + the terms of the GNU General Public License as published by the Free + Software Foundation; either version 3, or (at your option) any later + version. + + GCC is distributed in the hope that it will be useful, but WITHOUT ANY + WARRANTY; without even the implied warranty of MERCHANTABILITY or + FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + for more details. + + You should have received a copy of the GNU General Public License + along with GCC; see the file COPYING3. If not see + http://www.gnu.org/licenses/. */ Any libgcc source file (.c/.h etc., not makefiles) should have the runtime license exception. -- Joseph S. Myers jos...@codesourcery.com
Re: [Patch] libgcov.c re-factoring
On Fri, Jan 3, 2014 at 2:49 PM, Joseph S. Myers jos...@codesourcery.com wrote: On Fri, 3 Jan 2014, Teresa Johnson wrote: Index: libgcc/libgcov.h === --- libgcc/libgcov.h(revision 0) +++ libgcc/libgcov.h(revision 0) @@ -0,0 +1,224 @@ +/* Header file for libgcov-*.c. + Copyright (C) 1996-2013 Free Software Foundation, Inc. + + This file is part of GCC. + + GCC is free software; you can redistribute it and/or modify it under + the terms of the GNU General Public License as published by the Free + Software Foundation; either version 3, or (at your option) any later + version. + + GCC is distributed in the hope that it will be useful, but WITHOUT ANY + WARRANTY; without even the implied warranty of MERCHANTABILITY or + FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + for more details. + + You should have received a copy of the GNU General Public License + along with GCC; see the file COPYING3. If not see + http://www.gnu.org/licenses/. */ Any libgcc source file (.c/.h etc., not makefiles) should have the runtime license exception. Ok, thanks. I have changed this last paragraph to the following: Under Section 7 of GPL version 3, you are granted additional permissions described in the GCC Runtime Library Exception, version 3.1, as published by the Free Software Foundation. You should have received a copy of the GNU General Public License and a copy of the GCC Runtime Library Exception along with this program; see the files COPYING3 and COPYING.RUNTIME respectively. If not, see http://www.gnu.org/licenses/. */ Teresa -- Joseph S. Myers jos...@codesourcery.com -- Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
Re: [Patch] libgcov.c re-factoring
On Tue, Dec 17, 2013 at 7:48 AM, Teresa Johnson tejohn...@google.com wrote: On Mon, Dec 16, 2013 at 2:48 PM, Xinliang David Li davi...@google.com wrote: Ok -- gcov_write_counter and gcov_write_tag_length are qualified as low level primitives for basic gcov format and probably should be kept in gcov-io.c. gcov_rewrite is petty much libgcov runtime implementation details so I think it should be moved out. gcov_write_summary is not related to gcov low level format either, neither is gcov_seek. Ok for them to be moved? After looking at these some more, with the idea that gcov-io.c should encapsulate the lower level IO routines, then I think all of these (including gcov_rewrite) should remain in gcov-io.c. I think gcov_write_summary belongs there since all of the other gcov_write_* Yep, I think gcov_write_summary and read summary should not be split in between two directories. Similary for gcov_seek/rewrite I can see either the whole low-level I/O being abstracted away to -driver.c but currently -driver.c seem to containing higher level stuff that you apparenlty want to fork for kernel implementation instead and the actual i/o seems to remain in gcov-io.c +GCOV_LINKAGE struct gcov_var gcov_var; If gcov_var is not used by gcov-io.c only, why its declaration remains in gcov-io.h? Index: libgcc/libgcov.h === --- libgcc/libgcov.h(revision 0) +++ libgcc/libgcov.h(revision 0) @@ -0,0 +1,225 @@ +/* Header file for libgcov-*.c. + Contributed by Rong Xu x...@google.com. + Copyright (C) 2013 Free Software Foundation, Inc. I believe when the code was created by moving it from elsehwre, the copyright should say original date of gcov-io.h. + +#include tconfig.h +#include tsystem.h +#include coretypes.h +#include tm.h +#include libgcc_tm.h I would really like someone working on header restructuring to comment on those. I am not 100% sure what our best practices currently are in respect of including headers within headers and specially after good amount of defines like gcov-io.h gets included. + +#include gcov-io.h Otherwise the patch is OK (if header restructuring is fine and after moving gcov_var structure definition into gcov-io.c if possible). Honza
[Patch] libgcov.c re-factoring
On Tue, Dec 17, 2013 at 7:48 AM, Teresa Johnson tejohn...@google.com wrote:
On Mon, Dec 16, 2013 at 2:48 PM, Xinliang David Li davi...@google.com wrote:
Ok -- gcov_write_counter and gcov_write_tag_length are qualified as
low level primitives for basic gcov format and probably should be kept
in gcov-io.c.
gcov_rewrite is petty much libgcov runtime implementation details so I
think it should be moved out. gcov_write_summary is not related to
gcov low level format either, neither is gcov_seek. Ok for them to be
moved?
After looking at these some more, with the idea that gcov-io.c should
encapsulate the lower level IO routines, then I think all of these
(including gcov_rewrite) should remain in gcov-io.c. I think
gcov_write_summary belongs there since all of the other gcov_write_*
are there. And gcov_seek and gcov_rewrite are both adjusting gcov_var
fields to affect the file IO operations. And there are currently no
references to gcov_var within libgcc/libgcov* files.
So I think we should leave the patch as-is. Honza, is the current
patch ok for trunk?
Ping. Patch inlined below.
Thanks,
Teresa
2013-12-11 Rong Xu x...@google.com
* gcc/gcov-io.c (gcov_var): Move from gcov-io.h.
(gcov_position): Ditto.
(gcov_is_error): Ditto.
(gcov_rewrite): Ditto.
* gcc/gcov-io.h: Refactor. Move gcov_var to gcov-io.h, and libgcov
only part to libgcc/libgcov.h.
* libgcc/libgcov-driver.c: Use libgcov.h.
(buffer_fn_data): Use xmalloc instead of malloc.
(gcov_exit_merge_gcda): Ditto.
* libgcc/libgcov-driver-system.c (allocate_filename_struct): Ditto.
* libgcc/libgcov.h: New common header files for libgcov-*.h.
* libgcc/libgcov-interface.c: Use libgcov.h
* libgcc/libgcov-merge.c: Ditto.
* libgcc/libgcov-profiler.c: Ditto.
* libgcc/Makefile.in: Add dependence to libgcov.h
Index: gcc/gcov-io.c
===
--- gcc/gcov-io.c (revision 205895)
+++ gcc/gcov-io.c (working copy)
@@ -36,6 +36,36 @@ static const gcov_unsigned_t *gcov_read_words (uns
static void gcov_allocate (unsigned);
#endif
+GCOV_LINKAGE struct gcov_var gcov_var;
+
+/* Save the current position in the gcov file. */
+static inline gcov_position_t
+gcov_position (void)
+{
+ gcc_assert (gcov_var.mode 0);
+ return gcov_var.start + gcov_var.offset;
+}
+
+/* Return nonzero if the error flag is set. */
+static inline int
+gcov_is_error (void)
+{
+ return gcov_var.file ? gcov_var.error : 1;
+}
+
+#if IN_LIBGCOV
+/* Move to beginning of file and initialize for writing. */
+GCOV_LINKAGE inline void
+gcov_rewrite (void)
+{
+ gcc_assert (gcov_var.mode 0);
+ gcov_var.mode = -1;
+ gcov_var.start = 0;
+ gcov_var.offset = 0;
+ fseek (gcov_var.file, 0L, SEEK_SET);
+}
+#endif
+
static inline gcov_unsigned_t from_file (gcov_unsigned_t value)
{
#if !IN_LIBGCOV
Index: gcc/gcov-io.h
===
--- gcc/gcov-io.h (revision 205895)
+++ gcc/gcov-io.h (working copy)
@@ -164,51 +164,7 @@ see the files COPYING3 and COPYING.RUNTIME respect
#ifndef GCC_GCOV_IO_H
#define GCC_GCOV_IO_H
-#if IN_LIBGCOV
-/* About the target */
-
-#if BITS_PER_UNIT == 8
-typedef unsigned gcov_unsigned_t __attribute__ ((mode (SI)));
-typedef unsigned gcov_position_t __attribute__ ((mode (SI)));
-#if LONG_LONG_TYPE_SIZE 32
-typedef signed gcov_type __attribute__ ((mode (DI)));
-typedef unsigned gcov_type_unsigned __attribute__ ((mode (DI)));
-#else
-typedef signed gcov_type __attribute__ ((mode (SI)));
-typedef unsigned gcov_type_unsigned __attribute__ ((mode (SI)));
-#endif
-#else
-#if BITS_PER_UNIT == 16
-typedef unsigned gcov_unsigned_t __attribute__ ((mode (HI)));
-typedef unsigned gcov_position_t __attribute__ ((mode (HI)));
-#if LONG_LONG_TYPE_SIZE 32
-typedef signed gcov_type __attribute__ ((mode (SI)));
-typedef unsigned gcov_type_unsigned __attribute__ ((mode (SI)));
-#else
-typedef signed gcov_type __attribute__ ((mode (HI)));
-typedef unsigned gcov_type_unsigned __attribute__ ((mode (HI)));
-#endif
-#else
-typedef unsigned gcov_unsigned_t __attribute__ ((mode (QI)));
-typedef unsigned gcov_position_t __attribute__ ((mode (QI)));
-#if LONG_LONG_TYPE_SIZE 32
-typedef signed gcov_type __attribute__ ((mode (HI)));
-typedef unsigned gcov_type_unsigned __attribute__ ((mode (HI)));
-#else
-typedef signed gcov_type __attribute__ ((mode (QI)));
-typedef unsigned gcov_type_unsigned __attribute__ ((mode (QI)));
-#endif
-#endif
-#endif
-
-
-#if defined (TARGET_POSIX_IO)
-#define GCOV_LOCKED 1
-#else
-#define GCOV_LOCKED 0
-#endif
-
-#else /* !IN_LIBGCOV */
+#ifndef IN_LIBGCOV
/* About the host */
typedef unsigned gcov_unsigned_t;
@@ -231,48 +187,10 @@ typedef unsigned HOST_WIDEST_INT gcov_type_unsigne
#define GCOV_LOCKED 0
#endif
-#endif /* !IN_LIBGCOV */
-
-/* In gcov we want function linkage to be static. In the
