diff mbox series

glibc code coverage

Message ID CAF2FnvBig3eN83D-PhsbvH7oacxL48s4XU1g85==VWFCo5Cuuw@mail.gmail.com
State Changes Requested
Headers show
Series glibc code coverage | expand

Commit Message

Ashutosh Pandey Dec. 2, 2020, 10:42 a.m. UTC
I am a new contributor. I have applied for copyright papers and signed them.

Enable building glibc with instrumentation for code coverage. For
+    individual files '--coverage' can be used, but this fails
+    when it is used as an option with '../configure' command to build
glibc.
+    To successfully build glibc with coverage, you must first build and
install glibc
+    with shared libraries enabled. This will allow some files such as
+    'libc-modules.h' to be generated which are needed for code coverage to
+    work.
+    The command '../configure --disable-shared --enable-gcov
--without-selinux
+    --disable-nscd --prefix=/path/to/install'
+    will build glibc with code coverage. Note that not including a prefix,
+    or setting the prefix to usr/lib can affect the system glibc and can
+    render the system unusable. The command to make is 'make CXX=', without
+    which you will get a error 'cannot find -lgcc_s'.

Comments

Joseph Myers Dec. 2, 2020, 6:45 p.m. UTC | #1
On Wed, 2 Dec 2020, Ashutosh Pandey via Libc-alpha wrote:

> Enable building glibc with instrumentation for code coverage. For
> +    individual files '--coverage' can be used, but this fails
> +    when it is used as an option with '../configure' command to build
> glibc.
> +    To successfully build glibc with coverage, you must first build and
> install glibc
> +    with shared libraries enabled. This will allow some files such as
> +    'libc-modules.h' to be generated which are needed for code coverage to
> +    work.
> +    The command '../configure --disable-shared --enable-gcov
> --without-selinux
> +    --disable-nscd --prefix=/path/to/install'
> +    will build glibc with code coverage. Note that not including a prefix,
> +    or setting the prefix to usr/lib can affect the system glibc and can
> +    render the system unusable. The command to make is 'make CXX=', without
> +    which you will get a error 'cannot find -lgcc_s'.

Much of this reads more like a list of caveats about a work-in-progress 
patch, than a description of a configure option that is ready for 
inclusion in glibc.  I'd expect such issues to be fixed, so that the new 
option can be used by itself, before it should go in glibc.  If there is 
some reason it's inherently hard to fix those issues, the proposed commit 
message should explain *why* those issues arise and are hard to fix.

I know glibc has historically been hard to build and had many pitfalls in 
building it, but we should do better than that, which includes making new 
build-time features more robust.  (If something about the build 
environment doesn't work for a feature, a clear error message at configure 
time is much better than an obscure error part way through the build - 
whether or not that error part way through the build is mentioned in the 
installation manual.)

glibc should be configured using --prefix=/usr; a non-default glibc should 
be installed in a different sysroot, not configured with a different 
prefix.  (usr/lib isn't a prefix, it's a directory relative to a sysroot.)

Could you explain the design decisions made in more detail?  This patch 
looks like the configure option changes how the existing libraries are 
built.  Why is it doing things that way rather than something like 
--enable-profile: an option that enables an additional set of libraries 
built with coverage options, alongside the normal shared and static 
libraries?  Something like --enable-profile, that can be used with any 
other glibc configure options rather than needing a series of other 
options, including poorly-tested ones such as --disable-shared, could be 
more convenient for users - even if in practice it's more of an option for 
glibc developers than one where distributions would be expected to 
provided packages of the coverage-enabled libraries.

I suppose there's the question of whether coverage-enabled shared 
libraries make sense - if they do, it's harder to have them share a 
sysroot with normal shared libraries than it is to have a separate set of 
coverage-enabled static libraries.  So supporting coverage-enabled shared 
libraries would be one reason to have the configure option work as it does 
in this patch.

The documentation of the configure option in install.texi will need to 
make clear which choice is made (separate libraries versus changing how 
static / shared libraries are built).  Note that install.texi, saying what 
the option does, is separate from the commit message, which should discuss 
more about the rationale and design choices - separate text will be needed 
for each place.  A short NEWS entry will also be needed for a new 
configure option.  At least one build with the new option should be added 
to build-many-glibcs.py and verified to work there.

Note that configure should be regenerated with *unmodified* upstream 
autoconf, not with a version with distribution patches that introduce 
changes (--runstatedir) unrelated to the subject of the patch.
Ashutosh Pandey Feb. 4, 2021, 8:39 p.m. UTC | #2
On Thu, 3 Dec 2020, 12:15 am Joseph Myers, <joseph@codesourcery.com> wrote:

> On Wed, 2 Dec 2020, Ashutosh Pandey via Libc-alpha wrote:
>
> > Enable building glibc with instrumentation for code coverage. For
> > +    individual files '--coverage' can be used, but this fails
> > +    when it is used as an option with '../configure' command to build
> > glibc.
> > +    To successfully build glibc with coverage, you must first build and
> > install glibc
> > +    with shared libraries enabled. This will allow some files such as
> > +    'libc-modules.h' to be generated which are needed for code coverage
> to
> > +    work.
> > +    The command '../configure --disable-shared --enable-gcov
> > --without-selinux
> > +    --disable-nscd --prefix=/path/to/install'
> > +    will build glibc with code coverage. Note that not including a
> prefix,
> > +    or setting the prefix to usr/lib can affect the system glibc and can
> > +    render the system unusable. The command to make is 'make CXX=',
> without
> > +    which you will get a error 'cannot find -lgcc_s'.
>
> Much of this reads more like a list of caveats about a work-in-progress
> patch, than a description of a configure option that is ready for
> inclusion in glibc.  I'd expect such issues to be fixed, so that the new
> option can be used by itself, before it should go in glibc.  If there is
> some reason it's inherently hard to fix those issues, the proposed commit
> message should explain *why* those issues arise and are hard to fix.
>
> I know glibc has historically been hard to build and had many pitfalls in
> building it, but we should do better than that, which includes making new
> build-time features more robust.  (If something about the build
> environment doesn't work for a feature, a clear error message at configure
> time is much better than an obscure error part way through the build -
> whether or not that error part way through the build is mentioned in the
> installation manual.)
>
> glibc should be configured using --prefix=/usr; a non-default glibc should
> be installed in a different sysroot, not configured with a different
> prefix.  (usr/lib isn't a prefix, it's a directory relative to a sysroot.)
>
> Could you explain the design decisions made in more detail?  This patch
> looks like the configure option changes how the existing libraries are
> built.  Why is it doing things that way rather than something like
> --enable-profile: an option that enables an additional set of libraries
> built with coverage options, alongside the normal shared and static
> libraries?  Something like --enable-profile, that can be used with any
> other glibc configure options rather than needing a series of other
> options, including poorly-tested ones such as --disable-shared, could be
> more convenient for users - even if in practice it's more of an option for
> glibc developers than one where distributions would be expected to
> provided packages of the coverage-enabled libraries.
>
> I suppose there's the question of whether coverage-enabled shared
> libraries make sense - if they do, it's harder to have them share a
> sysroot with normal shared libraries than it is to have a separate set of
> coverage-enabled static libraries.  So supporting coverage-enabled shared
> libraries would be one reason to have the configure option work as it does
> in this patch.
>
> The documentation of the configure option in install.texi will need to
> make clear which choice is made (separate libraries versus changing how
> static / shared libraries are built).  Note that install.texi, saying what
> the option does, is separate from the commit message, which should discuss
> more about the rationale and design choices - separate text will be needed
> for each place.  A short NEWS entry will also be needed for a new
> configure option.  At least one build with the new option should be added
> to build-many-glibcs.py and verified to work there.
>
> Note that configure should be regenerated with *unmodified* upstream
> autoconf, not with a version with distribution patches that introduce
> changes (--runstatedir) unrelated to the subject of the patch.
>
> --
> Joseph S. Myers
> joseph@codesourcery.com


Hi,

Thank you for your feedback. I'm still working on improvements to the
patch. Progress is slow because I'm a beginner, but I'm still at it! In the
feedback you said:

I suppose there's the question of whether coverage-enabled shared
libraries make sense - if they do, it's harder to have them share a
sysroot with normal shared libraries than it is to have a separate set of
coverage-enabled static libraries.  So supporting coverage-enabled shared
libraries would be one reason to have the configure option work as it does
in this patch.

I actually gave a presentation at the recently concluded ELISA Workshop #6
where I discussed the patch, among other things. One of the attendees
suggested that I link it here, so I am doing that. Here's the Google drive
link:

https://drive.google.com/file/d/1eW3rz5vvvCTeCoLKZh72P1wPL3RYSYDp/view

I'm not sure about the other questions, but this should provide some
insight as to why this patch would be needed (atleast for a specific subset
of developers).

Once again, thank you for your time.

Regards,
Ashutosh
diff mbox series

Patch

From 3297a01fd688d35d5e242819688ba7e6150e77f2 Mon Sep 17 00:00:00 2001
From: Ashutosh Pandey <ashutoshpandey123456@gmail.com>
Date: Tue, 24 Nov 2020 00:20:03 +0530
Subject: [PATCH] glibc code coverage

---
 INSTALL             | 15 +++++++++++++++
 Makeconfig          |  7 +++++++
 config.make.in      |  1 +
 configure           | 28 +++++++++++++++++++++++++++-
 configure.ac        |  8 ++++++++
 manual/install.texi | 15 +++++++++++++++
 6 files changed, 73 insertions(+), 1 deletion(-)

diff --git a/INSTALL b/INSTALL
index 2b00f80df5..56923fbc8b 100644
--- a/INSTALL
+++ b/INSTALL
@@ -111,6 +111,21 @@  if 'CFLAGS' is specified it must enable optimization.  For example:
      systems support shared libraries; you need ELF support and
      (currently) the GNU linker.
 
+'--enable-gcov'
+    Enable building glibc with instrumentation for code coverage. For
+    individual files '--coverage' can be used, but this fails
+    when it is used as an option with '../configure' command to build glibc.
+    To successfully build glibc with coverage, you must first build and install glibc
+    with shared libraries enabled. This will allow some files such as
+    'libc-modules.h' to be generated which are needed for code coverage to
+    work.
+    The command '../configure --disable-shared --enable-gcov --without-selinux
+    --disable-nscd --prefix=/path/to/install'
+    will build glibc with code coverage. Note that not including a prefix,
+    or setting the prefix to usr/lib can affect the system glibc and can
+    render the system unusable. The command to make is 'make CXX=', without
+    which you will get a error 'cannot find -lgcc_s'.
+
 '--enable-static-pie'
      Enable static position independent executable (static PIE) support.
      Static PIE is similar to static executable, but can be loaded at
diff --git a/Makeconfig b/Makeconfig
index 8074613b85..5956de9d44 100644
--- a/Makeconfig
+++ b/Makeconfig
@@ -610,6 +610,13 @@  endif
 link-libc-static = -Wl,--start-group $(common-objpfx)libc.a $(static-gnulib) -Wl,--end-group
 link-libc-static-tests = -Wl,--start-group $(common-objpfx)libc.a $(static-gnulib-tests) -Wl,--end-group
 
+# support for code coverage with gcov
+ifeq (yes,$(build-gcov))
++cflags += -fprofile-arcs -ftest-coverage -O2
+link-libc-static = -Wl,--start-group -lgcov $(common-objpfx)libc.a $(static-gnulib) -Wl,--end-group
+link-libc-static-tests = -Wl,--start-group -lgcov $(common-objpfx)libc.a $(static-gnulib-tests) -Wl,--end-group
+endif
+
 # How to link against libgcc.  Some libgcc functions, such as those
 # for "long long" arithmetic or software floating point, can always be
 # built without use of C library headers and do not have any global
diff --git a/config.make.in b/config.make.in
index 1ac9417245..463e9d808c 100644
--- a/config.make.in
+++ b/config.make.in
@@ -92,6 +92,7 @@  build-shared = @shared@
 build-pic-default= @libc_cv_pic_default@
 build-pie-default= @libc_cv_pie_default@
 cc-pie-default= @libc_cv_cc_pie_default@
+build-gcov = @gcov@
 build-profile = @profile@
 build-static-nss = @static_nss@
 cross-compiling = @cross_compiling@
diff --git a/configure b/configure
index 4795e721e5..21f8582e16 100755
--- a/configure
+++ b/configure
@@ -683,6 +683,7 @@  force_install
 bindnow
 hardcoded_path_in_tests
 enable_timezone_tools
+gcov
 extra_nonshared_cflags
 use_default_link
 sysheaders
@@ -731,6 +732,7 @@  infodir
 docdir
 oldincludedir
 includedir
+runstatedir
 localstatedir
 sharedstatedir
 sysconfdir
@@ -765,6 +767,7 @@  with_default_link
 with_nonshared_cflags
 enable_sanity_checks
 enable_shared
+enable_gcov
 enable_profile
 enable_static_pie
 enable_timezone_tools
@@ -842,6 +845,7 @@  datadir='${datarootdir}'
 sysconfdir='${prefix}/etc'
 sharedstatedir='${prefix}/com'
 localstatedir='${prefix}/var'
+runstatedir='${localstatedir}/run'
 includedir='${prefix}/include'
 oldincludedir='/usr/include'
 docdir='${datarootdir}/doc/${PACKAGE_TARNAME}'
@@ -1094,6 +1098,15 @@  do
   | -silent | --silent | --silen | --sile | --sil)
     silent=yes ;;
 
+  -runstatedir | --runstatedir | --runstatedi | --runstated \
+  | --runstate | --runstat | --runsta | --runst | --runs \
+  | --run | --ru | --r)
+    ac_prev=runstatedir ;;
+  -runstatedir=* | --runstatedir=* | --runstatedi=* | --runstated=* \
+  | --runstate=* | --runstat=* | --runsta=* | --runst=* | --runs=* \
+  | --run=* | --ru=* | --r=*)
+    runstatedir=$ac_optarg ;;
+
   -sbindir | --sbindir | --sbindi | --sbind | --sbin | --sbi | --sb)
     ac_prev=sbindir ;;
   -sbindir=* | --sbindir=* | --sbindi=* | --sbind=* | --sbin=* \
@@ -1231,7 +1244,7 @@  fi
 for ac_var in	exec_prefix prefix bindir sbindir libexecdir datarootdir \
 		datadir sysconfdir sharedstatedir localstatedir includedir \
 		oldincludedir docdir infodir htmldir dvidir pdfdir psdir \
-		libdir localedir mandir
+		libdir localedir mandir runstatedir
 do
   eval ac_val=\$$ac_var
   # Remove trailing slashes.
@@ -1384,6 +1397,7 @@  Fine tuning of the installation directories:
   --sysconfdir=DIR        read-only single-machine data [PREFIX/etc]
   --sharedstatedir=DIR    modifiable architecture-independent data [PREFIX/com]
   --localstatedir=DIR     modifiable single-machine data [PREFIX/var]
+  --runstatedir=DIR       modifiable per-process data [LOCALSTATEDIR/run]
   --libdir=DIR            object code libraries [EPREFIX/lib]
   --includedir=DIR        C header files [PREFIX/include]
   --oldincludedir=DIR     C header files for non-gcc [/usr/include]
@@ -1420,6 +1434,8 @@  Optional Features:
   --disable-sanity-checks really do not use threads (should not be used except
                           in special situations) [default=yes]
   --enable-shared         build shared library [default=yes if GNU ld]
+  --enable-gcov           build with instrumentation for gcov code coverage
+                          [default=no]
   --enable-profile        build profiled library [default=no]
   --enable-static-pie     enable static PIE support and use it in the
                           testsuite [default=no]
@@ -3361,6 +3377,16 @@  else
   shared=yes
 fi
 
+
+# Check whether --enable-gcov was given.
+if test "${enable_gcov+set}" = set; then :
+  enableval=$enable_gcov; gcov=$enableval
+else
+  gcov=no
+fi
+
+
+
 # Check whether --enable-profile was given.
 if test "${enable_profile+set}" = set; then :
   enableval=$enable_profile; profile=$enableval
diff --git a/configure.ac b/configure.ac
index 93e68fb696..d5b27a4909 100644
--- a/configure.ac
+++ b/configure.ac
@@ -174,6 +174,14 @@  AC_ARG_ENABLE([shared],
 			     [build shared library @<:@default=yes if GNU ld@:>@]),
 	      [shared=$enableval],
 	      [shared=yes])
+
+AC_ARG_ENABLE([gcov],
+        AC_HELP_STRING([--enable-gcov],
+         [build with instrumentation for gcov code coverage library @<:@default=no@:>@]),
+           [gcov=$enableval],
+           [gcov=no])
+AC_SUBST(gcov)
+
 AC_ARG_ENABLE([profile],
 	      AC_HELP_STRING([--enable-profile],
 			     [build profiled library @<:@default=no@:>@]),
diff --git a/manual/install.texi b/manual/install.texi
index 2e164476d5..b990118ec6 100644
--- a/manual/install.texi
+++ b/manual/install.texi
@@ -141,6 +141,21 @@  Don't build shared libraries even if it is possible.  Not all systems
 support shared libraries; you need ELF support and (currently) the GNU
 linker.
 
+@item --enable-gcov
+Enable building glibc with instrumentation for code coverage. For
+individual files @option{--coverage} can be used, but this fails
+when it is used as an option with @code{configure} command to build glibc.
+To successfully build glibc with coverage, you must first build and install glibc
+with shared libraries enabled. This will allow some files such as
+@file{libc-modules.h} to be generated which are needed for code coverage
+to work.
+The command @samp{../configure --disable-shared --enable-gcov
+--without-selinux --disable-nscd --prefix=/path/to/install}
+will build glibc with code coverage. Note that not including a prefix,
+or setting the prefix to usr/lib can affect the system glibc and can
+render the system unusable. The command to make is 'make CXX=', without
+which you will get a error 'cannot find -lgcc_s'.
+
 @item --enable-static-pie
 Enable static position independent executable (static PIE) support.
 Static PIE is similar to static executable, but can be loaded at any
-- 
2.25.1