Re: [EXTERNAL] [PATCH] gcov: Use system IO buffering
On 6/17/21 6:21 PM, Eugene Rozenfeld wrote: Thank you for your reply Martin! AUTO_PROFILE_VERSION should also be changed. Then create_gcov can be updated to support both the old format and the new format. You are right. I've just pushed a commit https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=8819c82ce814a6911e2c1bfebd60b1c2366a3805 Martin Eugene -Original Message- From: Martin Liška Sent: Thursday, June 17, 2021 2:38 AM To: Eugene Rozenfeld ; gcc-patches@gcc.gnu.org Subject: Re: [EXTERNAL] [PATCH] gcov: Use system IO buffering On 6/17/21 3:59 AM, Eugene Rozenfeld wrote: |The commit from this patch (https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc.gnu.org%2Fgit%2F%3Fp%3Dgcc.git%3Ba%3Dcommit%3Bh%3D23eb66d1d46a34cb28c4acbdf8a1deb80a7c5a05data=04%7C01%7CEugene.Rozenfeld%40microsoft.com%7C508d63026ea84be211cc08d9317395bb%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637595194782996821%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=UG%2B41tXMZ94%2Ff80qCnmq%2BtZsFkLXc9NrdWF8KXwPjnk%3Dreserved=0) changed the semantics of gcov_read_string and gcov_write_string. Before this change the string storage was as described in gcov-io.h: "Strings are padded with 1 to 4 NUL bytes, to bring the length up to a multiple of 4. The number of 4 bytes is stored, followed by the padded string." After this change the number before the string indicates the number of bytes (not words) and there is no padding. Was this file format change intentional? Hello. Thanks for heads up! Yes, the change was intentional and I'm going to update documentation entry in gcov-io.h. It breaks AutoFDO because create_gcov produces strings in the format specified in gcov-io.h. Thanks, Eugene Sure, that needs to be adjusted. Martin
RE: [EXTERNAL] [PATCH] gcov: Use system IO buffering
Thank you for your reply Martin! AUTO_PROFILE_VERSION should also be changed. Then create_gcov can be updated to support both the old format and the new format. Eugene -Original Message- From: Martin Liška Sent: Thursday, June 17, 2021 2:38 AM To: Eugene Rozenfeld ; gcc-patches@gcc.gnu.org Subject: Re: [EXTERNAL] [PATCH] gcov: Use system IO buffering On 6/17/21 3:59 AM, Eugene Rozenfeld wrote: > |The commit from this patch > (https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc.gnu.org%2Fgit%2F%3Fp%3Dgcc.git%3Ba%3Dcommit%3Bh%3D23eb66d1d46a34cb28c4acbdf8a1deb80a7c5a05data=04%7C01%7CEugene.Rozenfeld%40microsoft.com%7C508d63026ea84be211cc08d9317395bb%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637595194782996821%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=UG%2B41tXMZ94%2Ff80qCnmq%2BtZsFkLXc9NrdWF8KXwPjnk%3Dreserved=0) > changed the semantics of gcov_read_string and gcov_write_string. Before this > change the string storage was as described in gcov-io.h: "Strings are padded > with 1 to 4 NUL bytes, to bring the length up to a multiple of 4. The number > of 4 bytes is stored, followed by the padded string." After this change the > number before the string indicates the number of bytes (not words) and there > is no padding. Was this file format change intentional? Hello. Thanks for heads up! Yes, the change was intentional and I'm going to update documentation entry in gcov-io.h. > It breaks AutoFDO because create_gcov produces strings in the format > specified in gcov-io.h. Thanks, Eugene Sure, that needs to be adjusted. Martin
Re: [EXTERNAL] [PATCH] gcov: Use system IO buffering
On 6/17/21 3:59 AM, Eugene Rozenfeld wrote: |The commit from this patch (https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=23eb66d1d46a34cb28c4acbdf8a1deb80a7c5a05) changed the semantics of gcov_read_string and gcov_write_string. Before this change the string storage was as described in gcov-io.h: "Strings are padded with 1 to 4 NUL bytes, to bring the length up to a multiple of 4. The number of 4 bytes is stored, followed by the padded string." After this change the number before the string indicates the number of bytes (not words) and there is no padding. Was this file format change intentional? Hello. Thanks for heads up! Yes, the change was intentional and I'm going to update documentation entry in gcov-io.h. It breaks AutoFDO because create_gcov produces strings in the format specified in gcov-io.h. Thanks, Eugene Sure, that needs to be adjusted. Martin
RE: [EXTERNAL] [PATCH] gcov: Use system IO buffering
The commit from this patch
(https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=23eb66d1d46a34cb28c4acbdf8a1deb80a7c5a05)
changed the semantics of gcov_read_string and gcov_write_string. Before this
change the string storage was as described in gcov-io.h:
"Strings are
padded with 1 to 4 NUL bytes, to bring the length up to a multiple
of 4. The number of 4 bytes is stored, followed by the padded
string."
After this change the number before the string indicates the number of bytes
(not words) and there is no padding.
Was this file format change intentional? It breaks AutoFDO because create_gcov
produces strings in the format specified in gcov-io.h.
Thanks,
Eugene
-Original Message-
From: Gcc-patches On Behalf Of Martin Liška
Sent: Wednesday, April 21, 2021 12:52 AM
To: gcc-patches@gcc.gnu.org
Subject: [EXTERNAL] [PATCH] gcov: Use system IO buffering
Hey.
I/O buffering in gcov seems duplicite to what modern C library can provide.
The patch is a simplification and can provide easier interface for system that
don't have a filesystem and would like using GCOV.
I'm going to install the patch after 11.1 if there are no objections.
Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
Thanks,
Martin
gcc/ChangeLog:
* gcov-io.c (gcov_write_block): Remove.
(gcov_write_words): Likewise.
(gcov_read_words): Re-implement using gcov_read_bytes.
(gcov_allocate): Remove.
(GCOV_BLOCK_SIZE): Likewise.
(struct gcov_var): Remove most of the fields.
(gcov_position): Implement with ftell.
(gcov_rewrite): Remove setting of start and offset fields.
(from_file): Re-format.
(gcov_open): Remove setbuf call. It should not be needed.
(gcov_close): Remove internal buffer handling.
(gcov_magic): Use __builtin_bswap32.
(gcov_write_counter): Use directly gcov_write_unsigned.
(gcov_write_string): Use direct fwrite and do not round
to 4 bytes.
(gcov_seek): Use directly fseek.
(gcov_write_tag): Use gcov_write_unsigned directly.
(gcov_write_length): Likewise.
(gcov_write_tag_length): Likewise.
(gcov_read_bytes): Use directly fread.
(gcov_read_unsigned): Use gcov_read_words.
(gcov_read_counter): Likewise.
(gcov_read_string): Use gcov_read_bytes.
* gcov-io.h (GCOV_WORD_SIZE): Adjust to reflect
that size is not in bytes, but words (4B).
(GCOV_TAG_FUNCTION_LENGTH): Likewise.
(GCOV_TAG_ARCS_LENGTH): Likewise.
(GCOV_TAG_ARCS_NUM): Likewise.
(GCOV_TAG_COUNTER_LENGTH): Likewise.
(GCOV_TAG_COUNTER_NUM): Likewise.
(GCOV_TAG_SUMMARY_LENGTH): Likewise.
libgcc/ChangeLog:
* libgcov-driver.c: Fix GNU coding style.
---
gcc/gcov-io.c | 282 +---
gcc/gcov-io.h | 17 ++-
libgcc/libgcov-driver.c | 6 +-
3 files changed, 76 insertions(+), 229 deletions(-)
diff --git a/gcc/gcov-io.c b/gcc/gcov-io.c index 80c9082a649..bd2316dedab 100644
--- a/gcc/gcov-io.c
+++ b/gcc/gcov-io.c
@@ -27,40 +27,14 @@ see the files COPYING3 and COPYING.RUNTIME respectively.
If not, see
/* Routines declared in gcov-io.h. This file should be #included by
another source file, after having #included gcov-io.h. */
-#if !IN_GCOV
-static void gcov_write_block (unsigned); -static gcov_unsigned_t
*gcov_write_words (unsigned); -#endif -static const gcov_unsigned_t
*gcov_read_words (unsigned); -#if !IN_LIBGCOV -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)
+static gcov_unsigned_t *gcov_read_words (void *buffer, unsigned);
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 */
+ int mode;/* < 0 writing, > 0 reading. */
int endian; /* Swap endianness. */
-#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
- /* 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. */ @@ -71,8 +45,7 @@ static
inline gcov_position_t gcov_position (void) {
-
Re: [PATCH] gcov: Use system IO buffering
On 4/23/21 11:44 AM, Richard Biener wrote: On Fri, Apr 23, 2021 at 11:24 AM Martin Liška wrote: On 4/23/21 8:49 AM, Richard Biener wrote: On Thu, Apr 22, 2021 at 9:47 PM Andi Kleen via Gcc-patches wrote: Martin Liška writes: Hey. I/O buffering in gcov seems duplicite to what modern C library can provide. The patch is a simplification and can provide easier interface for system that don't have a filesystem and would like using GCOV. I'm going to install the patch after 11.1 if there are no objections. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. What happens if someone compiles the C library with gcov? Haven't tried that.. Yeah, I think this is the wrong direction - we're already having issues with using malloc, this makes it much worse. I don't see where problem with my patch? It's not adding usage of any additional system routines. True. So the only impact should be more calls to libc (not necessarily more I/O since libc should do buffering). Exactly. "The patch is a simplification and can provide easier interface for system that don't have a filesystem and would like using GCOV." Can you explain? It's described in the following thread: https://gcc.gnu.org/pipermail/gcc-patches/2020-November/559342.html Martin Martin Richard. Being as self contained as possible (only system calls) would seem safer. -Andi
Re: [PATCH] gcov: Use system IO buffering
On Fri, Apr 23, 2021 at 11:24 AM Martin Liška wrote: > > On 4/23/21 8:49 AM, Richard Biener wrote: > > On Thu, Apr 22, 2021 at 9:47 PM Andi Kleen via Gcc-patches > > wrote: > >> > >> Martin Liška writes: > >> > >>> Hey. > >>> > >>> I/O buffering in gcov seems duplicite to what modern C library can > >>> provide. > >>> The patch is a simplification and can provide easier interface for system > >>> that don't have a filesystem and would like using GCOV. > >>> > >>> I'm going to install the patch after 11.1 if there are no objections. > >>> > >>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > >> > >> What happens if someone compiles the C library with gcov? > > Haven't tried that.. > > > > > Yeah, I think this is the wrong direction - we're already having > > issues with using > > malloc, this makes it much worse. > > I don't see where problem with my patch? It's not adding usage of any > additional > system routines. True. So the only impact should be more calls to libc (not necessarily more I/O since libc should do buffering). "The patch is a simplification and can provide easier interface for system that don't have a filesystem and would like using GCOV." Can you explain? > Martin > > > > > Richard. > > > >> Being as self contained as possible (only system calls) would seem > >> safer. > >> > >> -Andi >
Re: [PATCH] gcov: Use system IO buffering
On 4/23/21 8:49 AM, Richard Biener wrote: > On Thu, Apr 22, 2021 at 9:47 PM Andi Kleen via Gcc-patches > wrote: >> >> Martin Liška writes: >> >>> Hey. >>> >>> I/O buffering in gcov seems duplicite to what modern C library can provide. >>> The patch is a simplification and can provide easier interface for system >>> that don't have a filesystem and would like using GCOV. >>> >>> I'm going to install the patch after 11.1 if there are no objections. >>> >>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> >> What happens if someone compiles the C library with gcov? Haven't tried that.. > > Yeah, I think this is the wrong direction - we're already having > issues with using > malloc, this makes it much worse. I don't see where problem with my patch? It's not adding usage of any additional system routines. Martin > > Richard. > >> Being as self contained as possible (only system calls) would seem >> safer. >> >> -Andi
Re: [PATCH] gcov: Use system IO buffering
On Thu, Apr 22, 2021 at 9:47 PM Andi Kleen via Gcc-patches wrote: > > Martin Liška writes: > > > Hey. > > > > I/O buffering in gcov seems duplicite to what modern C library can provide. > > The patch is a simplification and can provide easier interface for system > > that don't have a filesystem and would like using GCOV. > > > > I'm going to install the patch after 11.1 if there are no objections. > > > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > What happens if someone compiles the C library with gcov? Yeah, I think this is the wrong direction - we're already having issues with using malloc, this makes it much worse. Richard. > Being as self contained as possible (only system calls) would seem > safer. > > -Andi
Re: [PATCH] gcov: Use system IO buffering
Martin Liška writes: > Hey. > > I/O buffering in gcov seems duplicite to what modern C library can provide. > The patch is a simplification and can provide easier interface for system > that don't have a filesystem and would like using GCOV. > > I'm going to install the patch after 11.1 if there are no objections. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. What happens if someone compiles the C library with gcov? Being as self contained as possible (only system calls) would seem safer. -Andi
[PATCH] gcov: Use system IO buffering
Hey.
I/O buffering in gcov seems duplicite to what modern C library can provide.
The patch is a simplification and can provide easier interface for system
that don't have a filesystem and would like using GCOV.
I'm going to install the patch after 11.1 if there are no objections.
Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
Thanks,
Martin
gcc/ChangeLog:
* gcov-io.c (gcov_write_block): Remove.
(gcov_write_words): Likewise.
(gcov_read_words): Re-implement using gcov_read_bytes.
(gcov_allocate): Remove.
(GCOV_BLOCK_SIZE): Likewise.
(struct gcov_var): Remove most of the fields.
(gcov_position): Implement with ftell.
(gcov_rewrite): Remove setting of start and offset fields.
(from_file): Re-format.
(gcov_open): Remove setbuf call. It should not be needed.
(gcov_close): Remove internal buffer handling.
(gcov_magic): Use __builtin_bswap32.
(gcov_write_counter): Use directly gcov_write_unsigned.
(gcov_write_string): Use direct fwrite and do not round
to 4 bytes.
(gcov_seek): Use directly fseek.
(gcov_write_tag): Use gcov_write_unsigned directly.
(gcov_write_length): Likewise.
(gcov_write_tag_length): Likewise.
(gcov_read_bytes): Use directly fread.
(gcov_read_unsigned): Use gcov_read_words.
(gcov_read_counter): Likewise.
(gcov_read_string): Use gcov_read_bytes.
* gcov-io.h (GCOV_WORD_SIZE): Adjust to reflect
that size is not in bytes, but words (4B).
(GCOV_TAG_FUNCTION_LENGTH): Likewise.
(GCOV_TAG_ARCS_LENGTH): Likewise.
(GCOV_TAG_ARCS_NUM): Likewise.
(GCOV_TAG_COUNTER_LENGTH): Likewise.
(GCOV_TAG_COUNTER_NUM): Likewise.
(GCOV_TAG_SUMMARY_LENGTH): Likewise.
libgcc/ChangeLog:
* libgcov-driver.c: Fix GNU coding style.
---
gcc/gcov-io.c | 282 +---
gcc/gcov-io.h | 17 ++-
libgcc/libgcov-driver.c | 6 +-
3 files changed, 76 insertions(+), 229 deletions(-)
diff --git a/gcc/gcov-io.c b/gcc/gcov-io.c
index 80c9082a649..bd2316dedab 100644
--- a/gcc/gcov-io.c
+++ b/gcc/gcov-io.c
@@ -27,40 +27,14 @@ see the files COPYING3 and COPYING.RUNTIME respectively.
If not, see
/* Routines declared in gcov-io.h. This file should be #included by
another source file, after having #included gcov-io.h. */
-#if !IN_GCOV
-static void gcov_write_block (unsigned);
-static gcov_unsigned_t *gcov_write_words (unsigned);
-#endif
-static const gcov_unsigned_t *gcov_read_words (unsigned);
-#if !IN_LIBGCOV
-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)
+static gcov_unsigned_t *gcov_read_words (void *buffer, unsigned);
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 */
+ int mode;/* < 0 writing, > 0 reading. */
int endian; /* Swap endianness. */
-#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
- /* 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. */
@@ -71,8 +45,7 @@ static inline
gcov_position_t
gcov_position (void)
{
- gcov_nonruntime_assert (gcov_var.mode > 0);
- return gcov_var.start + gcov_var.offset;
+ return ftell (gcov_var.file);
}
/* Return nonzero if the error flag is set. */
@@ -92,20 +65,16 @@ GCOV_LINKAGE inline void
gcov_rewrite (void)
{
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)
+static inline gcov_unsigned_t
+from_file (gcov_unsigned_t value)
{
#if !IN_LIBGCOV || defined (IN_GCOV_TOOL)
if (gcov_var.endian)
-{
- value = (value >> 16) | (value << 16);
- value = ((value & 0xff00ff) << 8) | ((value >> 8) & 0xff00ff);
-}
+return __builtin_bswap32 (value);
#endif
return value;
}
@@ -140,9 +109,6 @@ gcov_open (const char *name, int mode)
#endif
gcov_nonruntime_assert (!gcov_var.file);
- gcov_var.start = 0;
- gcov_var.offset =
