[PATCH RFC 00/11] Add Memory Sanitizer support

[PATCH RFC 00/11] Add Memory Sanitizer support

[Ilya Leoshkevich]

Hi,

This series adds minimalistic support for Memory Sanitizer (MSan) [1].
MSan is compiler instrumentation for detecting accesses to
uninitialized memory.

The motivation behind this is to be able to link elfutils into projects
instrumented with MSan, since it essentially requires all the code
running in a process to be instrumented.

The goal is to provide a setup where elfutils is linked only with zlib
and most tests pass. Here is the description of the setup that I'm
using:

- LLVM with argp_parse() instrumentation [2].

- zlib-ng instrumented with MSan:

  git clone git@github.com:zlib-ng/zlib-ng.git
  cmake -DWITH_SANITIZER=Memory -DZLIB_COMPAT=ON -DWITH_GTEST=OFF \
        -DCMAKE_C_COMPILER=clang -DCMAKE_INSTALL_PREFIX=/tmp/zlib-ng
  make install
  export CPATH=/tmp/zlib-ng/include
  export LIBRARY_PATH=/tmp/zlib-ng/lib

- Hack: zlib is used by a lot of system utilities, so adding
  MSan-instrumented zlib to LD_LIBRARY_PATH causes a lot of grief.
  Let elfutils test infrastructure add it there only for running
  tests:

  ln -s /tmp/zlib-ng/lib/libz.so.1 libelf/

- elfutils uses printf("%n"), so tweak MSan to unpoison the respective
  arguments. Also disable fast unwinding to get better backtraces:

  export MSAN_OPTIONS=check_printf=1,fast_unwind_on_malloc=0

- Minimal configuration of elfutils instrumented with MSan:

  autoreconf -i
  CC=clang ./configure --enable-maintainer-mode \
                       --enable-sanitize-memory --without-bzlib \
                       --without-lzma --without-zstd \
                       --disable-debuginfod --disable-libdebuginfod \
                       --disable-demangler

Results:

  ============================================================================
  Testsuite summary for elfutils 0.188
  ============================================================================
  # TOTAL: 235
  # PASS:  221
  # SKIP:  14
  # XFAIL: 0
  # FAIL:  0
  # XPASS: 0
  # ERROR: 0
  ============================================================================

The patches take care of the following:

- Fixing clang build.
- Adding small tweaks to get rid of false positives (no real issues
  were found, most likely because elfutils is already tested with
  valgrind).
- Dealing with "-self" tests, which now see MSan runtime compiled
  into elfutils binaries.
- MSan enablement itself.

[1] https://clang.llvm.org/docs/MemorySanitizer.html
[2] https://reviews.llvm.org/D143330

Best regards,
Ilya

Ilya Leoshkevich (11):
  libdwfl: Fix debuginfod_client redefinition
  libasm: Fix xdefault_pattern initialization
  printversion: Fix unused variable
  readelf: Fix set but not used parameter
  readelf: Fix set but not used variable
  Initialize reglocs for VMCOREINFO
  addr2line: Do not test demangling in run-addr2line-i-test.sh
  x86_64_return_value_location: Support lvalue and rvalue references
  configure: Use -fno-addrsig if possible
  configure: Add --disable-demangle
  configure: Add --enable-sanitize-memory

 backends/linux-core-note.c    |  1 +
 backends/x86_64_retval.c      |  2 ++
 configure.ac                  | 40 ++++++++++++++++++++++++++++++++++-
 debuginfod/Makefile.am        |  3 ++-
 lib/printversion.h            |  3 ++-
 libasm/Makefile.am            |  3 ++-
 libasm/asm_newscn.c           |  5 ++---
 libdw/Makefile.am             |  3 ++-
 libdwfl/debuginfod-client.c   |  2 +-
 libdwfl/libdwfl.h             |  5 +----
 libdwfl/libdwflP.h            |  4 ++--
 libelf/Makefile.am            |  3 ++-
 src/readelf.c                 |  3 +--
 tests/Makefile.am             | 10 ++++++++-
 tests/run-addr2line-i-test.sh | 14 ++++++------
 tests/run-readelf-self.sh     |  5 +++++
 tests/run-strip-reloc.sh      |  5 +++++
 tests/run-varlocs-self.sh     |  5 +++++
 18 files changed, 90 insertions(+), 26 deletions(-)

-- 
2.39.1

[PATCH RFC 01/11] libdwfl: Fix debuginfod_client redefinition

[Ilya Leoshkevich]

clang complains:

    In file included from debuginfod-client.c:38:
    ./../debuginfod/debuginfod.h:47:34: error: redefinition of typedef 'debuginfod_client' is a C11 feature [-Werror,-Wtypedef-redefinition]
    typedef struct debuginfod_client debuginfod_client;
                                     ^
    ./libdwfl.h:53:34: note: previous definition is here
    typedef struct debuginfod_client debuginfod_client;
                                     ^

config/eu.am specifies -std=gnu99, and upgrading just for this is an
overkill. So is #including "debuginfod.h", since we don't know if users
even have it. So fix by using "struct debuginfod_client" instead. This
may break the clients that use dwfl_get_debuginfod_client() without
#including "debuginfod.h", but such cases should be rare.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 libdwfl/debuginfod-client.c | 2 +-
 libdwfl/libdwfl.h           | 5 +----
 libdwfl/libdwflP.h          | 4 ++--
 3 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/libdwfl/debuginfod-client.c b/libdwfl/debuginfod-client.c
index 882a5eff..c6f8e083 100644
--- a/libdwfl/debuginfod-client.c
+++ b/libdwfl/debuginfod-client.c
@@ -143,7 +143,7 @@ __libdwfl_debuginfod_init (void)
 
 #else // ENABLE_LIBDEBUGINFOD
 
-debuginfod_client *
+struct debuginfod_client *
 dwfl_get_debuginfod_client (Dwfl *dummy __attribute__ ((unused)))
 {
   return NULL;
diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
index 9114f7f0..ba2f081f 100644
--- a/libdwfl/libdwfl.h
+++ b/libdwfl/libdwfl.h
@@ -49,9 +49,6 @@ typedef struct Dwfl_Thread Dwfl_Thread;
    PC location described by an FDE belonging to Dwfl_Thread.  */
 typedef struct Dwfl_Frame Dwfl_Frame;
 
-/* Handle for debuginfod-client connection.  */
-typedef struct debuginfod_client debuginfod_client;
-
 /* Callbacks.  */
 typedef struct
 {
@@ -813,7 +810,7 @@ int dwfl_frame_reg (Dwfl_Frame *state, unsigned regno, Dwarf_Word *val)
    first call to this function. If elfutils is compiled without support for debuginfod,
    NULL will be returned.
  */
-extern debuginfod_client *dwfl_get_debuginfod_client (Dwfl *dwfl);
+extern struct debuginfod_client *dwfl_get_debuginfod_client (Dwfl *dwfl);
 
 #ifdef __cplusplus
 }
diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h
index cdc528d0..fbd66f45 100644
--- a/libdwfl/libdwflP.h
+++ b/libdwfl/libdwflP.h
@@ -114,7 +114,7 @@ struct Dwfl
 {
   const Dwfl_Callbacks *callbacks;
 #ifdef ENABLE_LIBDEBUGINFOD
-  debuginfod_client *debuginfod;
+  struct debuginfod_client *debuginfod;
 #endif
   Dwfl_Module *modulelist;    /* List in order used by full traversals.  */
 
@@ -652,7 +652,7 @@ __libdwfl_debuginfod_find_debuginfo (Dwfl *dwfl,
 				     const unsigned char *build_id_bits,
 				     size_t build_id_len);
 void
-__libdwfl_debuginfod_end (debuginfod_client *c);
+__libdwfl_debuginfod_end (struct debuginfod_client *c);
 #endif
 
 
-- 
2.39.1

[PATCH RFC 02/11] libasm: Fix xdefault_pattern initialization

[Ilya Leoshkevich]

clang complains:

    asm_newscn.c:48:22: error: field 'pattern' with variable sized type 'struct FillPattern' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
      struct FillPattern pattern;
                         ^

Fix by using a union instead. Define the second union member to be a
char array 1 byte larger than struct FillPattern. This should be legal
according to 6.7.9:

    If an object that has static or thread storage duration is not
    initialized explicitly, then ... if it is a union, the first named
    member is initialized (recursively) according to these rules, and
    any padding is initialized to zero bits.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 libasm/asm_newscn.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/libasm/asm_newscn.c b/libasm/asm_newscn.c
index d258d969..32a3b598 100644
--- a/libasm/asm_newscn.c
+++ b/libasm/asm_newscn.c
@@ -43,17 +43,16 @@
 /* Memory for the default pattern.  The type uses a flexible array
    which does work well with a static initializer.  So we play some
    dirty tricks here.  */
-static const struct
+static const union
 {
   struct FillPattern pattern;
-  char zero;
+  char zeroes[sizeof(struct FillPattern) + 1];
 } xdefault_pattern =
   {
     .pattern =
     {
       .len = 1
     },
-    .zero = '\0'
   };
 const struct FillPattern *__libasm_default_pattern = &xdefault_pattern.pattern;
 
-- 
2.39.1

[PATCH RFC 03/11] printversion: Fix unused variable

[Ilya Leoshkevich]

clang complains:

    debuginfod.cxx:354:1: error: unused variable 'apba__' [-Werror,-Wunused-const-variable]
    ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT;
    ^
    ../lib/printversion.h:47:21: note: expanded from macro 'ARGP_PROGRAM_BUG_ADDRESS_DEF'
      const char *const apba__ __asm ("argp_program_bug_address")
                        ^

This is as expected: it's used by argp via the
"argp_program_bug_address" name, which is not visible on the C level.
Add __attribute__ ((used)) to make sure that the compiler emits it.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 lib/printversion.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/printversion.h b/lib/printversion.h
index a9e059ff..d1c3a987 100644
--- a/lib/printversion.h
+++ b/lib/printversion.h
@@ -44,6 +44,7 @@ void print_version (FILE *stream, struct argp_state *state);
   void (*const apvh) (FILE *, struct argp_state *) \
    __asm ("argp_program_version_hook")
 #define ARGP_PROGRAM_BUG_ADDRESS_DEF \
-  const char *const apba__ __asm ("argp_program_bug_address")
+  const char *const apba__ __asm ("argp_program_bug_address") \
+  __attribute__ ((used))
 
 #endif // PRINTVERSION_H
-- 
2.39.1

[PATCH RFC 04/11] readelf: Fix set but not used parameter

[Ilya Leoshkevich]

clang complains:

    readelf.c:12205:72: error: parameter 'desc' set but not used [-Werror,-Wunused-but-set-parameter]
    handle_bit_registers (const Ebl_Register_Location *regloc, const void *desc,
                                                                           ^

Apparently handle_bit_registers() is unimplemented, but one line is
still written for the future. Silence the warning by casting desc to
void.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 src/readelf.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/readelf.c b/src/readelf.c
index 51b0e8b9..f09c5c9b 100644
--- a/src/readelf.c
+++ b/src/readelf.c
@@ -12206,6 +12206,7 @@ handle_bit_registers (const Ebl_Register_Location *regloc, const void *desc,
 		      unsigned int colno)
 {
   desc += regloc->offset;
+  (void)desc;
 
   abort ();			/* XXX */
   return colno;
-- 
2.39.1

[PATCH RFC 05/11] readelf: Fix set but not used variable

[Ilya Leoshkevich]

clang complains:

    readelf.c:10250:10: error: variable 'nculist' set but not used [-Werror,-Wunused-but-set-variable]
      size_t nculist = 0;
             ^

Fix by deleting it.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 src/readelf.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/readelf.c b/src/readelf.c
index f09c5c9b..76ca65f5 100644
--- a/src/readelf.c
+++ b/src/readelf.c
@@ -10247,7 +10247,6 @@ print_debug_macro_section (Dwfl_Module *dwflmod __attribute__ ((unused)),
   Dwarf_Off ncu = 0;
   size_t hsize;
   struct mac_culist *culist = NULL;
-  size_t nculist = 0;
   while (dwarf_nextcu (dbg, offset = ncu, &ncu, &hsize, NULL, NULL, NULL) == 0)
     {
       Dwarf_Die cudie;
@@ -10268,7 +10267,6 @@ print_debug_macro_section (Dwfl_Module *dwflmod __attribute__ ((unused)),
       newp->files = NULL;
       newp->next = culist;
       culist = newp;
-      ++nculist;
     }
 
   const unsigned char *readp = (const unsigned char *) data->d_buf;
-- 
2.39.1

[PATCH RFC 06/11] Initialize reglocs for VMCOREINFO

[Ilya Leoshkevich]

MSan complains:

    Uninitialized value was created by an allocation of 'reglocs' in the stack frame
       #0 0x562d35c686f0 in handle_core_note elfutils/src/readelf.c:12674:3
       #const Ebl_Register_Location *reglocs;
    ==1006199==WARNING: MemorySanitizer: use-of-uninitialized-value
       #0 0x562d35c68a2a in handle_core_note elfutils/src/readelf.c:12692:11
       #colno = handle_core_registers (ebl, ebl->elf, desc + regs_offset,
       #                               reglocs, nregloc);

Strictly speaking, this is not a problem, because nregloc == 0, but for
other note types we initialize it anyway, so do it here as well.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 backends/linux-core-note.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/backends/linux-core-note.c b/backends/linux-core-note.c
index 9faae4c3..238ec16d 100644
--- a/backends/linux-core-note.c
+++ b/backends/linux-core-note.c
@@ -239,6 +239,7 @@ EBLHOOK(core_note) (const GElf_Nhdr *nhdr, const char *name,
 	return 0;
       *regs_offset = 0;
       *nregloc = 0;
+      *reglocs = NULL;
       *nitems = 1;
       *items = vmcoreinfo_items;
       return 1;
-- 
2.39.1

[PATCH RFC 07/11] addr2line: Do not test demangling in run-addr2line-i-test.sh

[Ilya Leoshkevich]

There is run-addr2line-i-demangle-test.sh for that.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/run-addr2line-i-test.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/run-addr2line-i-test.sh b/tests/run-addr2line-i-test.sh
index 4f63e487..e7b89083 100755
--- a/tests/run-addr2line-i-test.sh
+++ b/tests/run-addr2line-i-test.sh
@@ -254,13 +254,13 @@ EOF
 
 testfiles testfile-inlines-lto
 
-testrun_compare ${abs_top_builddir}/src/addr2line --pretty -fiC -e testfile-inlines-lto 0x1118 0x1137 <<\EOF
-foobar(int) at /tmp/x.cpp:4:14
- (inlined by) foo(int) at /tmp/x.cpp:22:16
- (inlined by) fu(int) at /tmp/x.cpp:27:13
-fubar(int) at /tmp/x.cpp:10:14
- (inlined by) bar(int) at /tmp/x.cpp:16:15
- (inlined by) fu(int) at /tmp/x.cpp:27:24
+testrun_compare ${abs_top_builddir}/src/addr2line --pretty -fi -e testfile-inlines-lto 0x1118 0x1137 <<\EOF
+_Z6foobari at /tmp/x.cpp:4:14
+ (inlined by) _Z3fooi at /tmp/x.cpp:22:16
+ (inlined by) _Z2fui at /tmp/x.cpp:27:13
+_Z5fubari at /tmp/x.cpp:10:14
+ (inlined by) _Z3bari at /tmp/x.cpp:16:15
+ (inlined by) _Z2fui at /tmp/x.cpp:27:24
 EOF
 
 exit 0
-- 
2.39.1

[PATCH RFC 08/11] x86_64_return_value_location: Support lvalue and rvalue references

[Ilya Leoshkevich]

On the low level, they are the same as pointers.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 backends/x86_64_retval.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/backends/x86_64_retval.c b/backends/x86_64_retval.c
index f9114cb1..e668eacc 100644
--- a/backends/x86_64_retval.c
+++ b/backends/x86_64_retval.c
@@ -106,6 +106,8 @@ x86_64_return_value_location (Dwarf_Die *functypedie, const Dwarf_Op **locp)
     case DW_TAG_enumeration_type:
     case DW_TAG_pointer_type:
     case DW_TAG_ptr_to_member_type:
+    case DW_TAG_reference_type:
+    case DW_TAG_rvalue_reference_type:
       {
 	Dwarf_Attribute attr_mem;
 	if (dwarf_formudata (dwarf_attr_integrate (typedie, DW_AT_byte_size,
-- 
2.39.1

[PATCH RFC 09/11] configure: Use -fno-addrsig if possible

[Ilya Leoshkevich]

By default, clang produces .llvm_addrsig sections [1]. The GNU
toolchain does not know how to handle them yet [2], so just ask clang
not to generate them for the time being.

[1] https://llvm.org/docs/Extensions.html#sht-llvm-addrsig-section-address-significance-table
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105625

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 configure.ac | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/configure.ac b/configure.ac
index 8fe8baee..7dc9be63 100644
--- a/configure.ac
+++ b/configure.ac
@@ -588,6 +588,14 @@ CFLAGS="$old_CFLAGS"])
 AM_CONDITIONAL(HAVE_NO_PACKED_NOT_ALIGNED_WARNING,
 	       [test "x$ac_cv_no_packed_not_aligned" != "xno"])
 
+AC_CACHE_CHECK([whether the compiler accepts -fno-addrsig], ac_cv_fno_addrsig, [dnl
+old_CFLAGS="$CFLAGS"
+CFLAGS="$CFLAGS -fno-addrsig -Werror"
+AC_COMPILE_IFELSE([AC_LANG_SOURCE([])],
+                  ac_cv_fno_addrsig=yes, ac_cv_fno_addrsig=no)
+CFLAGS="$old_CFLAGS"])
+AS_IF([test "x$ac_cv_fno_addrsig" = "xyes"], CFLAGS="$CFLAGS -fno-addrsig")
+
 saved_LIBS="$LIBS"
 AC_SEARCH_LIBS([argp_parse], [argp])
 LIBS="$saved_LIBS"
-- 
2.39.1

[PATCH RFC 10/11] configure: Add --disable-demangle

[Ilya Leoshkevich]

__cxa_demangle is normally implemented in the C++ runtime library,
instrumenting which for MSan is a hassle. Add a knob for disabling it.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 configure.ac | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 7dc9be63..6a5c38af 100644
--- a/configure.ac
+++ b/configure.ac
@@ -466,11 +466,17 @@ CFLAGS="$CFLAGS -D_GNU_SOURCE"
 AC_FUNC_STRERROR_R()
 CFLAGS="$old_CFLAGS"
 
+AC_ARG_ENABLE([demangler],
+AS_HELP_STRING([--disable-demangle],
+               [Disable libstdc++ demangle support]))
+
+AS_IF([test "x$enable_demangler" == xyes],
 AC_CHECK_LIB([stdc++], [__cxa_demangle], [dnl
 AC_DEFINE([USE_DEMANGLE], [1], [Defined if demangling is enabled])])
 AM_CONDITIONAL(DEMANGLE, test "x$ac_cv_lib_stdcpp___cxa_demangle" = "xyes")
 AS_IF([test "x$ac_cv_lib_stdcpp___cxa_demangle" = "xyes"],
-      [enable_demangler=yes],[enable_demangler=no])
+      [enable_demangler=yes],[enable_demangler=no]),
+AM_CONDITIONAL(DEMANGLE, false))
 
 AC_ARG_ENABLE([textrelcheck],
 AS_HELP_STRING([--disable-textrelcheck],
-- 
2.39.1

[PATCH RFC 11/11] configure: Add --enable-sanitize-memory

[Ilya Leoshkevich]

Add support for clang Memory Sanitizer [1], which detects the usage of
uninitialized values. While elfutils itself is already checked with
valgrind, checking code that depends on elfutils requires elfutils to
be built with MSan.

MSan is not linked into shared libraries, and is linked into
executables statically. Therefore, unlike the other sanitizers, MSan
needs to be configured fairly early, since we need to drop
-D_FORTIFY_SOURCE [2], -Wl,-z,defs and --no-undefined.

Disable a few tests that run for more than 5 minutes due to test files
being statically linked with MSan.

[1] https://clang.llvm.org/docs/MemorySanitizer.html
[2] https://github.com/google/sanitizers/issues/247

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 configure.ac              | 24 ++++++++++++++++++++++++
 debuginfod/Makefile.am    |  3 ++-
 libasm/Makefile.am        |  3 ++-
 libdw/Makefile.am         |  3 ++-
 libelf/Makefile.am        |  3 ++-
 tests/Makefile.am         | 10 +++++++++-
 tests/run-readelf-self.sh |  5 +++++
 tests/run-strip-reloc.sh  |  5 +++++
 tests/run-varlocs-self.sh |  5 +++++
 9 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/configure.ac b/configure.ac
index 6a5c38af..86e156a0 100644
--- a/configure.ac
+++ b/configure.ac
@@ -155,6 +155,29 @@ AC_SUBST([fpie_CFLAGS])
 
 dso_LDFLAGS="-shared"
 
+NO_UNDEFINED=-Wl,--no-undefined
+AC_ARG_ENABLE([sanitize-memory],
+              AS_HELP_STRING([--enable-sanitize-memory],
+                             [Use clang memory sanitizer]),
+                             [use_msan=$enableval], [use_msan=no])
+if test "$use_msan" = yes; then
+  old_CFLAGS="$CFLAGS"
+  old_CXXFLAGS="$CXXFLAGS"
+  old_LDFLAGS="$LDFLAGS"
+  # -fsanitize=memory is not compatible with -D_FORTIFY_SOURCE, -Wl,-z,defs and --no-undefined
+  CFLAGS="$CFLAGS -fsanitize=memory -fsanitize-memory-track-origins -D_FORTIFY_SOURCE=0"
+  CXXFLAGS="$CXXFLAGS -fsanitize=memory -fsanitize-memory-track-origins -D_FORTIFY_SOURCE=0"
+  LDFLAGS="-shared"
+  AC_LINK_IFELSE([AC_LANG_SOURCE([int main (int argc, char **argv) { return 0; }])], use_msan=yes, use_msan=no)
+  AS_IF([test "x$use_msan" == xyes],
+        ac_cv_zdefs=no NO_UNDEFINED=,
+        AC_MSG_WARN([clang memory sanitizer not available])
+        CFLAGS="$old_CFLAGS" CXXFLAGS="$old_CXXFLAGS")
+  LDFLAGS="$old_LDFLAGS"
+fi
+AC_SUBST(NO_UNDEFINED)
+AM_CONDITIONAL(USE_MEMORY_SANITIZER, test "$use_msan" = yes)
+
 ZDEFS_LDFLAGS="-Wl,-z,defs"
 AC_CACHE_CHECK([whether gcc supports $ZDEFS_LDFLAGS], ac_cv_zdefs, [dnl
 save_LDFLAGS="$LDFLAGS"
@@ -887,6 +910,7 @@ AC_MSG_NOTICE([
     run all tests under valgrind       : ${use_valgrind}
     gcc undefined behaviour sanitizer  : ${use_undefined}
     gcc address sanitizer              : ${use_address}
+    clang memory sanitizer             : ${use_msan}
     use rpath in tests                 : ${tests_use_rpath}
     test biarch                        : ${utrace_cv_cc_biarch}
 ])
diff --git a/debuginfod/Makefile.am b/debuginfod/Makefile.am
index f27d6e2e..125be97b 100644
--- a/debuginfod/Makefile.am
+++ b/debuginfod/Makefile.am
@@ -102,7 +102,8 @@ endif
 $(LIBDEBUGINFOD_SONAME): $(srcdir)/libdebuginfod.map $(libdebuginfod_so_LIBS)
 	$(AM_V_CCLD)$(LINK) $(dso_LDFLAGS) -o $@ \
 		-Wl,--soname,$(LIBDEBUGINFOD_SONAME) \
-		-Wl,--version-script,$<,--no-undefined \
+		-Wl,--version-script,$< \
+		$(NO_UNDEFINED) \
 		-Wl,--whole-archive $(libdebuginfod_so_LIBS) -Wl,--no-whole-archive \
 		$(libdebuginfod_so_LDLIBS)
 	@$(textrel_check)
diff --git a/libasm/Makefile.am b/libasm/Makefile.am
index c2b54811..1e6b63e8 100644
--- a/libasm/Makefile.am
+++ b/libasm/Makefile.am
@@ -64,7 +64,8 @@ libasm_so_LIBS = libasm_pic.a
 libasm.so: $(srcdir)/libasm.map $(libasm_so_LIBS) $(libasm_so_DEPS)
 	$(AM_V_CCLD)$(LINK) $(dso_LDFLAGS) -o $@ \
 		-Wl,--soname,$@.$(VERSION) \
-		-Wl,--version-script,$<,--no-undefined \
+		-Wl,--version-script,$< \
+		$(NO_UNDEFINED) \
 		-Wl,--whole-archive $(libasm_so_LIBS) -Wl,--no-whole-archive \
 		$(libasm_so_LDLIBS)
 	@$(textrel_check)
diff --git a/libdw/Makefile.am b/libdw/Makefile.am
index 1b6fead4..e548f38c 100644
--- a/libdw/Makefile.am
+++ b/libdw/Makefile.am
@@ -114,7 +114,8 @@ libdw_so_LDLIBS = $(libdw_so_DEPS) -ldl -lz $(argp_LDADD) $(fts_LIBS) $(obstack_
 libdw.so: $(srcdir)/libdw.map $(libdw_so_LIBS) $(libdw_so_DEPS)
 	$(AM_V_CCLD)$(LINK) $(dso_LDFLAGS) -o $@ \
 		-Wl,--soname,$@.$(VERSION),--enable-new-dtags \
-		-Wl,--version-script,$<,--no-undefined \
+		-Wl,--version-script,$< \
+		$(NO_UNDEFINED) \
 		-Wl,--whole-archive $(libdw_so_LIBS) -Wl,--no-whole-archive \
 		$(libdw_so_LDLIBS)
 	@$(textrel_check)
diff --git a/libelf/Makefile.am b/libelf/Makefile.am
index 24c25cf8..aabce43e 100644
--- a/libelf/Makefile.am
+++ b/libelf/Makefile.am
@@ -115,7 +115,8 @@ libelf_so_LIBS = libelf_pic.a
 libelf.so: $(srcdir)/libelf.map $(libelf_so_LIBS) $(libelf_so_DEPS)
 	$(AM_V_CCLD)$(LINK) $(dso_LDFLAGS) -o $@ \
 		-Wl,--soname,$@.$(VERSION) \
-		-Wl,--version-script,$<,--no-undefined \
+		-Wl,--version-script,$< \
+		$(NO_UNDEFINED) \
 		-Wl,--whole-archive $(libelf_so_LIBS) -Wl,--no-whole-archive \
 		$(libelf_so_LDLIBS)
 	@$(textrel_check)
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 36823d94..31dd2f67 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -88,12 +88,16 @@ endif
 
 # test_nlist checks its own symbol table, and expects various symbols
 # to be in the order as specified in the source file. Explicitly set
-# minimal CFLAGS. But add address sanitizer if in use.
+# minimal CFLAGS. But add sanitizers if in use.
 if USE_ADDRESS_SANITIZER
 EXTRA_NLIST_CFLAGS=-fsanitize=address
 else
+if USE_MEMORY_SANITIZER
+EXTRA_NLIST_CFLAGS=-fsanitize=memory -fsanitize-memory-track-origins
+else
 EXTRA_NLIST_CFLAGS=
 endif
+endif
 
 test-nlist$(EXEEXT): test-nlist.c
 	$(AM_V_CC)$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) \
@@ -225,6 +229,10 @@ if USE_ZSTD_COMPRESS
 export ELFUTILS_ZSTD = 1
 endif
 
+if USE_MEMORY_SANITIZER
+export ELFUTILS_MEMORY_SANITIZER = 1
+endif
+
 if DEBUGINFOD
 check_PROGRAMS += debuginfod_build_id_find
 # With the dummy delegation doesn't work
diff --git a/tests/run-readelf-self.sh b/tests/run-readelf-self.sh
index 7ffb3577..61f803fb 100755
--- a/tests/run-readelf-self.sh
+++ b/tests/run-readelf-self.sh
@@ -17,5 +17,10 @@
 
 . $srcdir/test-subr.sh
 
+if test -n "$ELFUTILS_MEMORY_SANITIZER"; then
+  echo "binaries statically linked memory sanitizer are too big"
+  exit 77
+fi
+
 # Just makes sure readelf doesn't crash
 testrun_on_self_quiet ${abs_top_builddir}/src/readelf -a -w
diff --git a/tests/run-strip-reloc.sh b/tests/run-strip-reloc.sh
index 033ed278..31a11fa2 100755
--- a/tests/run-strip-reloc.sh
+++ b/tests/run-strip-reloc.sh
@@ -17,6 +17,11 @@
 
 . $srcdir/test-subr.sh
 
+if test -n "$ELFUTILS_MEMORY_SANITIZER"; then
+  echo "binaries statically linked memory sanitizer are too big"
+  exit 77
+fi
+
 testfiles hello_i386.ko hello_x86_64.ko hello_ppc64.ko hello_s390.ko \
 	hello_aarch64.ko hello_m68k.ko hello_riscv64.ko hello_csky.ko \
 	hello_arc_hs4.ko
diff --git a/tests/run-varlocs-self.sh b/tests/run-varlocs-self.sh
index 5454fc70..7d79f70e 100755
--- a/tests/run-varlocs-self.sh
+++ b/tests/run-varlocs-self.sh
@@ -17,6 +17,11 @@
 
 . $srcdir/test-subr.sh
 
+if test -n "$ELFUTILS_MEMORY_SANITIZER"; then
+  echo "binaries statically linked memory sanitizer are too big"
+  exit 77
+fi
+
 # Make sure varlocs doesn't crash, doesn't trigger self-check/asserts
 # or leaks running under valgrind.
 testrun_on_self_exe ${abs_top_builddir}/tests/varlocs -e
-- 
2.39.1

Re: [PATCH RFC 00/11] Add Memory Sanitizer support

[Mark Wielaard]

Hi Ilya,

On Mon, Feb 06, 2023 at 11:25:02PM +0100, Ilya Leoshkevich via Elfutils-devel wrote:
> This series adds minimalistic support for Memory Sanitizer (MSan) [1].
> MSan is compiler instrumentation for detecting accesses to
> uninitialized memory.
> 
> The motivation behind this is to be able to link elfutils into projects
> instrumented with MSan, since it essentially requires all the code
> running in a process to be instrumented.

Interesting. For regular CI testing we do use ubsan, valgrind and/or
asan. So msan might not find many new issues in the elfutils code
itself. But being able to link the elfutils libraries instrumented with
msan against other projects build with msan might be very useful.

> The goal is to provide a setup where elfutils is linked only with zlib
> and most tests pass. Here is the description of the setup that I'm
> using:
> 
> - LLVM with argp_parse() instrumentation [2].
> 
> - zlib-ng instrumented with MSan:
> 
>   git clone git@github.com:zlib-ng/zlib-ng.git
>   cmake -DWITH_SANITIZER=Memory -DZLIB_COMPAT=ON -DWITH_GTEST=OFF \
>         -DCMAKE_C_COMPILER=clang -DCMAKE_INSTALL_PREFIX=/tmp/zlib-ng
>   make install
>   export CPATH=/tmp/zlib-ng/include
>   export LIBRARY_PATH=/tmp/zlib-ng/lib
> 
> - Hack: zlib is used by a lot of system utilities, so adding
>   MSan-instrumented zlib to LD_LIBRARY_PATH causes a lot of grief.
>   Let elfutils test infrastructure add it there only for running
>   tests:
> 
>   ln -s /tmp/zlib-ng/lib/libz.so.1 libelf/
> 
> - elfutils uses printf("%n"), so tweak MSan to unpoison the respective
>   arguments. Also disable fast unwinding to get better backtraces:
> 
>   export MSAN_OPTIONS=check_printf=1,fast_unwind_on_malloc=0
> 
> - Minimal configuration of elfutils instrumented with MSan:
> 
>   autoreconf -i
>   CC=clang ./configure --enable-maintainer-mode \
>                        --enable-sanitize-memory --without-bzlib \
>                        --without-lzma --without-zstd \
>                        --disable-debuginfod --disable-libdebuginfod \
>                        --disable-demangler

Aren't there instrumented versions of bzip2, lzma/xz and/or zstd?

Can't debuginfod and libdebuginfod be instrumented?

Is the demangler disabled because you don't link against (an
instrumented) libstdc++?

> Results:
> 
>   ============================================================================
>   Testsuite summary for elfutils 0.188
>   ============================================================================
>   # TOTAL: 235
>   # PASS:  221
>   # SKIP:  14
>   # XFAIL: 0
>   # FAIL:  0
>   # XPASS: 0
>   # ERROR: 0
>   ============================================================================

Very good.

> The patches take care of the following:
> 
> - Fixing clang build.

Yeah, it is a pity msan hasn't been integrated with gcc, we often find
issues with clang.

> - Adding small tweaks to get rid of false positives (no real issues
>   were found, most likely because elfutils is already tested with
>   valgrind).
> - Dealing with "-self" tests, which now see MSan runtime compiled
>   into elfutils binaries.
> - MSan enablement itself.
> 
> Ilya Leoshkevich (11):
>   libdwfl: Fix debuginfod_client redefinition
>   libasm: Fix xdefault_pattern initialization
>   printversion: Fix unused variable
>   readelf: Fix set but not used parameter
>   readelf: Fix set but not used variable
>   Initialize reglocs for VMCOREINFO
>   addr2line: Do not test demangling in run-addr2line-i-test.sh
>   x86_64_return_value_location: Support lvalue and rvalue references
>   configure: Use -fno-addrsig if possible
>   configure: Add --disable-demangle
>   configure: Add --enable-sanitize-memory

Thanks for splitting things out so nicely in separate patches.

Cheers,

Mark

Re: [PATCH RFC 01/11] libdwfl: Fix debuginfod_client redefinition

[Mark Wielaard]

Hi Ilyam

On Mon, Feb 06, 2023 at 11:25:03PM +0100, Ilya Leoshkevich via Elfutils-devel wrote:
> clang complains:
> 
>     In file included from debuginfod-client.c:38:
>     ./../debuginfod/debuginfod.h:47:34: error: redefinition of typedef 'debuginfod_client' is a C11 feature [-Werror,-Wtypedef-redefinition]
>     typedef struct debuginfod_client debuginfod_client;
>                                      ^
>     ./libdwfl.h:53:34: note: previous definition is here
>     typedef struct debuginfod_client debuginfod_client;
>                                      ^
> 
> config/eu.am specifies -std=gnu99, and upgrading just for this is an
> overkill. So is #including "debuginfod.h", since we don't know if users
> even have it. So fix by using "struct debuginfod_client" instead. This
> may break the clients that use dwfl_get_debuginfod_client() without
> #including "debuginfod.h", but such cases should be rare.

This was recently reported by someone else and fixed differently:

commit 45576ab5f24cd39669a418fa8e005b4d04f8e9ca
Author: Mark Wielaard <mark@klomp.org>
Date:   Mon Feb 6 10:21:58 2023 +0100

    debuginfod: Make sure there is only one typedef for debuginfod_client
    
    Both debuginfod.h and libdwfl.h have a simple typedef for struct
    debuginfod_client. Some compilers pedantically warn when including
    both headers that such typedefs are only officially supported in
    C11. So guard them with _ELFUTILS_DEBUGINFOD_CLIENT_TYPEDEF to
    make them happy.
    
    https://sourceware.org/bugzilla/show_bug.cgi?id=30077
    
    Signed-off-by: Mark Wielaard <mark@klomp.org>

Does that work for you?

Thanks,

Mark

Re: [PATCH RFC 02/11] libasm: Fix xdefault_pattern initialization

[Mark Wielaard]

Hi Ilya,

On Mon, Feb 06, 2023 at 11:25:04PM +0100, Ilya Leoshkevich via Elfutils-devel wrote:
> clang complains:
> 
>     asm_newscn.c:48:22: error: field 'pattern' with variable sized type 'struct FillPattern' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
>       struct FillPattern pattern;
>                          ^
> 
> Fix by using a union instead. Define the second union member to be a
> char array 1 byte larger than struct FillPattern. This should be legal
> according to 6.7.9:
> 
>     If an object that has static or thread storage duration is not
>     initialized explicitly, then ... if it is a union, the first named
>     member is initialized (recursively) according to these rules, and
>     any padding is initialized to zero bits.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  libasm/asm_newscn.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/libasm/asm_newscn.c b/libasm/asm_newscn.c
> index d258d969..32a3b598 100644
> --- a/libasm/asm_newscn.c
> +++ b/libasm/asm_newscn.c
> @@ -43,17 +43,16 @@
>  /* Memory for the default pattern.  The type uses a flexible array
>     which does work well with a static initializer.  So we play some
>     dirty tricks here.  */
> -static const struct
> +static const union
>  {
>    struct FillPattern pattern;
> -  char zero;
> +  char zeroes[sizeof(struct FillPattern) + 1];
>  } xdefault_pattern =
>    {
>      .pattern =
>      {
>        .len = 1
>      },
> -    .zero = '\0'
>    };

Yes, I think this works. Could you update the comment just before this
with some of the commit message explanation? Your explanation is much
better than "play some dirty trick" :)

>  const struct FillPattern *__libasm_default_pattern = &xdefault_pattern.pattern;

I am surprised this doesn't need a cast. Do you know why?

Thanks,

Mark

Re: [PATCH RFC 00/11] Add Memory Sanitizer support

[Ilya Leoshkevich]

On Tue, 2023-02-07 at 20:05 +0100, Mark Wielaard wrote:
> Hi Ilya,
> 
> On Mon, Feb 06, 2023 at 11:25:02PM +0100, Ilya Leoshkevich via
> Elfutils-devel wrote:
> > This series adds minimalistic support for Memory Sanitizer (MSan)
> > [1].
> > MSan is compiler instrumentation for detecting accesses to
> > uninitialized memory.

[...]

> > - Minimal configuration of elfutils instrumented with MSan:
> > 
> >   autoreconf -i
> >   CC=clang ./configure --enable-maintainer-mode \
> >                        --enable-sanitize-memory --without-bzlib \
> >                        --without-lzma --without-zstd \
> >                        --disable-debuginfod --disable-libdebuginfod
> > \
> >                        --disable-demangler
> 
> Aren't there instrumented versions of bzip2, lzma/xz and/or zstd?
> 
> Can't debuginfod and libdebuginfod be instrumented?
> 
> Is the demangler disabled because you don't link against (an
> instrumented) libstdc++?

I think with some effort instrumenting the dependencies is possible.
bzlib and lzma are not particularly large, and zstd should support
this out of the box. Regarding C++, an instrumented LLVM's libc++
should also just work. With all this, it should be possible to test
elfutils with MSan without disabling the extra functionality.

But since you already test with valgrind, I figured it would be highly
unlikely that I find new bugs, and decided to limit the scope here.
For my current purposes - linking elfutils into libbpf - this proved
to be enough.

[...]

Best regards,
Ilya

Re: [PATCH RFC 01/11] libdwfl: Fix debuginfod_client redefinition

[Ilya Leoshkevich]

On Tue, 2023-02-07 at 20:22 +0100, Mark Wielaard wrote:
> Hi Ilyam
> 
> On Mon, Feb 06, 2023 at 11:25:03PM +0100, Ilya Leoshkevich via
> Elfutils-devel wrote:
> > clang complains:
> > 
> >     In file included from debuginfod-client.c:38:
> >     ./../debuginfod/debuginfod.h:47:34: error: redefinition of
> > typedef 'debuginfod_client' is a C11 feature [-Werror,-Wtypedef-
> > redefinition]
> >     typedef struct debuginfod_client debuginfod_client;
> >                                      ^
> >     ./libdwfl.h:53:34: note: previous definition is here
> >     typedef struct debuginfod_client debuginfod_client;
> >                                      ^
> > 
> > config/eu.am specifies -std=gnu99, and upgrading just for this is
> > an
> > overkill. So is #including "debuginfod.h", since we don't know if
> > users
> > even have it. So fix by using "struct debuginfod_client" instead.
> > This
> > may break the clients that use dwfl_get_debuginfod_client() without
> > #including "debuginfod.h", but such cases should be rare.
> 
> This was recently reported by someone else and fixed differently:
> 
> commit 45576ab5f24cd39669a418fa8e005b4d04f8e9ca
> Author: Mark Wielaard <mark@klomp.org>
> Date:   Mon Feb 6 10:21:58 2023 +0100
> 
>     debuginfod: Make sure there is only one typedef for
> debuginfod_client
>     
>     Both debuginfod.h and libdwfl.h have a simple typedef for struct
>     debuginfod_client. Some compilers pedantically warn when
> including
>     both headers that such typedefs are only officially supported in
>     C11. So guard them with _ELFUTILS_DEBUGINFOD_CLIENT_TYPEDEF to
>     make them happy.
>     
>     https://sourceware.org/bugzilla/show_bug.cgi?id=30077
>     
>     Signed-off-by: Mark Wielaard <mark@klomp.org>
> 
> Does that work for you?
> 
> Thanks,
> 
> Mark

Thanks, this works. This patch can be dropped.

Re: [PATCH RFC 02/11] libasm: Fix xdefault_pattern initialization

[Ilya Leoshkevich]

On Tue, 2023-02-07 at 20:41 +0100, Mark Wielaard wrote:
> Hi Ilya,
> 
> On Mon, Feb 06, 2023 at 11:25:04PM +0100, Ilya Leoshkevich via
> Elfutils-devel wrote:
> > clang complains:
> > 
> >     asm_newscn.c:48:22: error: field 'pattern' with variable sized
> > type 'struct FillPattern' not at the end of a struct or class is a
> > GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> >       struct FillPattern pattern;
> >                          ^
> > 
> > Fix by using a union instead. Define the second union member to be
> > a
> > char array 1 byte larger than struct FillPattern. This should be
> > legal
> > according to 6.7.9:
> > 
> >     If an object that has static or thread storage duration is not
> >     initialized explicitly, then ... if it is a union, the first
> > named
> >     member is initialized (recursively) according to these rules,
> > and
> >     any padding is initialized to zero bits.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >  libasm/asm_newscn.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libasm/asm_newscn.c b/libasm/asm_newscn.c
> > index d258d969..32a3b598 100644
> > --- a/libasm/asm_newscn.c
> > +++ b/libasm/asm_newscn.c
> > @@ -43,17 +43,16 @@
> >  /* Memory for the default pattern.  The type uses a flexible array
> >     which does work well with a static initializer.  So we play
> > some
> >     dirty tricks here.  */
> > -static const struct
> > +static const union
> >  {
> >    struct FillPattern pattern;
> > -  char zero;
> > +  char zeroes[sizeof(struct FillPattern) + 1];
> >  } xdefault_pattern =
> >    {
> >      .pattern =
> >      {
> >        .len = 1
> >      },
> > -    .zero = '\0'
> >    };
> 
> Yes, I think this works. Could you update the comment just before
> this
> with some of the commit message explanation? Your explanation is much
> better than "play some dirty trick" :)

Thanks, will do.

> >  const struct FillPattern *__libasm_default_pattern =
> > &xdefault_pattern.pattern;
> 
> I am surprised this doesn't need a cast. Do you know why?

We are referencing the union's .pattern member, not the entire union,
so the types still match.

> 
> Thanks,
> 
> Mark

Re: [PATCH RFC 03/11] printversion: Fix unused variable

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 2370 bytes --]

Hi Ilya (CC Frank),

On Mon, Feb 06, 2023 at 11:25:05PM +0100, Ilya Leoshkevich via Elfutils-devel wrote:
> clang complains:
> 
>     debuginfod.cxx:354:1: error: unused variable 'apba__' [-Werror,-Wunused-const-variable]
>     ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT;
>     ^
>     ../lib/printversion.h:47:21: note: expanded from macro 'ARGP_PROGRAM_BUG_ADDRESS_DEF'
>       const char *const apba__ __asm ("argp_program_bug_address")
>                         ^
> 
> This is as expected: it's used by argp via the
> "argp_program_bug_address" name, which is not visible on the C level.
> Add __attribute__ ((used)) to make sure that the compiler emits it.

Actually I think it found a real issue. Note that the same construct
is used the C eu tools.  But in debuginfod.cxx it says:

/* Name and version of program.  */
/* ARGP_PROGRAM_VERSION_HOOK_DEF = print_version; */ // not this simple for C++

/* Bug report address.  */
ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT;

Note how ARGP_PROGRAM_VERSION_HOOK_DEF is commented out and in main it
has:

   /* Parse and process arguments.  */
   int remaining;
   argp_program_version_hook = print_version; // this works
   (void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER, &remaining, NULL);

So it sets print_version, but not argp_program_bug_address.
And indeed debuginfod --help is missing the bug reporting address.

I don't really know/understand why the printversion.h macro trick doesn't work with C++ (symbol mangling?). But we do need at least this patch:

diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index 4271acf4..0ec326d5 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -4172,6 +4165,7 @@ main (int argc, char *argv[])
   /* Parse and process arguments.  */
   int remaining;
   argp_program_version_hook = print_version; // this works
+  argp_program_bug_address = PACKAGE_BUGREPORT;
   (void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER, &remaining, NULL);
   if (remaining != argc)
       error (EXIT_FAILURE, 0,

Then debuginfod --help will say: Report bugs to
https://sourceware.org/bugzilla.

That of course doesn't help with the -Wunused-const-variable warning.

If we cannot figure out the magic variable naming trick with with C++
then maybe we can just not include printversion.h and do it "by hand"?
(as attached)

Cheers,

Mark

[-- Attachment #2: debuginfod-version.patch --]
[-- Type: text/plain, Size: 1221 bytes --]

diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index 4271acf4..1ea90645 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -45,7 +45,6 @@ extern "C" {
 #endif
 
 extern "C" {
-#include "printversion.h"
 #include "system.h"
 }
 
@@ -345,13 +344,11 @@ static const char DEBUGINFOD_SQLITE_CLEANUP_DDL[] =
   ;
 
 
-
-
-/* Name and version of program.  */
-/* ARGP_PROGRAM_VERSION_HOOK_DEF = print_version; */ // not this simple for C++
-
-/* Bug report address.  */
-ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT;
+extern "C" {
+/* Defined in version.c.  Explicitly declared here because including
+   print_version.h magic variable tricks don't work in C++.  */
+void print_version (FILE *stream, struct argp_state *state);
+}
 
 /* Definitions of arguments for argp functions.  */
 static const struct argp_option options[] =
@@ -4172,6 +4169,7 @@ main (int argc, char *argv[])
   /* Parse and process arguments.  */
   int remaining;
   argp_program_version_hook = print_version; // this works
+  argp_program_bug_address = PACKAGE_BUGREPORT;
   (void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER, &remaining, NULL);
   if (remaining != argc)
       error (EXIT_FAILURE, 0,

Re: [PATCH RFC 03/11] printversion: Fix unused variable

[Ilya Leoshkevich]

On Tue, 2023-02-07 at 21:44 +0100, Mark Wielaard wrote:
> Hi Ilya (CC Frank),
> 
> On Mon, Feb 06, 2023 at 11:25:05PM +0100, Ilya Leoshkevich via
> Elfutils-devel wrote:
> > clang complains:
> > 
> >     debuginfod.cxx:354:1: error: unused variable 'apba__' [-
> > Werror,-Wunused-const-variable]
> >     ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT;
> >     ^
> >     ../lib/printversion.h:47:21: note: expanded from macro
> > 'ARGP_PROGRAM_BUG_ADDRESS_DEF'
> >       const char *const apba__ __asm ("argp_program_bug_address")
> >                         ^
> > 
> > This is as expected: it's used by argp via the
> > "argp_program_bug_address" name, which is not visible on the C
> > level.
> > Add __attribute__ ((used)) to make sure that the compiler emits it.
> 
> Actually I think it found a real issue. Note that the same construct
> is used the C eu tools.  But in debuginfod.cxx it says:
> 
> /* Name and version of program.  */
> /* ARGP_PROGRAM_VERSION_HOOK_DEF = print_version; */ // not this
> simple for C++
> 
> /* Bug report address.  */
> ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT;
> 
> Note how ARGP_PROGRAM_VERSION_HOOK_DEF is commented out and in main
> it
> has:
> 
>    /* Parse and process arguments.  */
>    int remaining;
>    argp_program_version_hook = print_version; // this works
>    (void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER, &remaining,
> NULL);
> 
> So it sets print_version, but not argp_program_bug_address.
> And indeed debuginfod --help is missing the bug reporting address.
> 
> I don't really know/understand why the printversion.h macro trick
> doesn't work with C++ (symbol mangling?). But we do need at least
> this patch:
> 
> diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
> index 4271acf4..0ec326d5 100644
> --- a/debuginfod/debuginfod.cxx
> +++ b/debuginfod/debuginfod.cxx
> @@ -4172,6 +4165,7 @@ main (int argc, char *argv[])
>    /* Parse and process arguments.  */
>    int remaining;
>    argp_program_version_hook = print_version; // this works
> +  argp_program_bug_address = PACKAGE_BUGREPORT;
>    (void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER, &remaining,
> NULL);
>    if (remaining != argc)
>        error (EXIT_FAILURE, 0,
> 
> Then debuginfod --help will say: Report bugs to
> https://sourceware.org/bugzilla.
> 
> That of course doesn't help with the -Wunused-const-variable warning.
> 
> If we cannot figure out the magic variable naming trick with with C++
> then maybe we can just not include printversion.h and do it "by
> hand"?
> (as attached)
> 
> Cheers,
> 
> Mark

If I build:

const char *const apba__ __asm ("argp_program_bug_address") \
__attribute__ ((used)) = "foobarbaz";

with C and C++, the difference is going to be:

@@ -1,6 +1,5 @@
 	.file	"1.c"
 	.text
-	.globl	argp_program_bug_address
 	.section	.rodata.str1.1,"aMS",@progbits,1
 .LC0:
 	.string	"foobarbaz"

This must have to do with C and C++ standards treating const
differently [1]. The solution is to add extern:

--- a/lib/printversion.h
+++ b/lib/printversion.h
@@ -44,6 +44,7 @@ void print_version (FILE *stream, struct argp_state
*state);
   void (*const apvh) (FILE *, struct argp_state *) \
    __asm ("argp_program_version_hook")
 #define ARGP_PROGRAM_BUG_ADDRESS_DEF \
+  extern const char *const apba__; \
   const char *const apba__ __asm ("argp_program_bug_address") \
   __attribute__ ((used))

I can include this in v2 if it works for you.

[1]
https://stackoverflow.com/questions/8908071/const-correctness-in-c-vs-c

Re: [PATCH RFC 04/11] readelf: Fix set but not used parameter

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1207 bytes --]

Hi Ilya,

On Mon, 2023-02-06 at 23:25 +0100, Ilya Leoshkevich via Elfutils-devel
wrote:
> clang complains:
> 
>     readelf.c:12205:72: error: parameter 'desc' set but not used [-Werror,-Wunused-but-set-parameter]
>     handle_bit_registers (const Ebl_Register_Location *regloc, const void *desc,
>                                                                            ^
> 
> Apparently handle_bit_registers() is unimplemented, but one line is
> still written for the future. Silence the warning by casting desc to
> void.

Someone else also noticed this and filed a bug report, could you add
the bug URL to the commit message?

https://sourceware.org/bugzilla/show_bug.cgi?id=30084

Also can we just remove this whole function?

It is never really used since as far as I can see we don't have any
backend with a core register sets where a register doesn't have a
number of bits which isn't a multiple of 8 (only ia64 has some 1 bit
registers, but those don't seem part of the core register set).

If we do accidentally try to handle such a register having an abort is
also not very nice. Lets just warn and return/continue. Something like
the attached?

Thanks,

Mark

[-- Attachment #2: p --]
[-- Type: text/x-patch, Size: 863 bytes --]

diff --git a/src/readelf.c b/src/readelf.c
index 51b0e8b9..50bfd1c8 100644
--- a/src/readelf.c
+++ b/src/readelf.c
@@ -12201,24 +12201,17 @@ handle_core_items (Elf *core, const void *desc, size_t descsz,
   return colno;
 }
 
-static unsigned int
-handle_bit_registers (const Ebl_Register_Location *regloc, const void *desc,
-		      unsigned int colno)
-{
-  desc += regloc->offset;
-
-  abort ();			/* XXX */
-  return colno;
-}
-
-
 static unsigned int
 handle_core_register (Ebl *ebl, Elf *core, int maxregname,
 		      const Ebl_Register_Location *regloc, const void *desc,
 		      unsigned int colno)
 {
   if (regloc->bits % 8 != 0)
-    return handle_bit_registers (regloc, desc, colno);
+    {
+      error (0, 0, "Warning: Cannot handle register with %" PRIu8 "bits\n",
+	     regloc->bits);
+      return colno;
+    }
 
   desc += regloc->offset;
 

Re: [PATCH RFC 05/11] readelf: Fix set but not used variable

[Mark Wielaard]

Hi Ilya,

On Mon, 2023-02-06 at 23:25 +0100, Ilya Leoshkevich via Elfutils-devel
wrote:
> clang complains:
> 
>     readelf.c:10250:10: error: variable 'nculist' set but not used [-Werror,-Wunused-but-set-variable]
>       size_t nculist = 0;
>              ^
> 
> Fix by deleting it.

yeah, this is clearly a copy/paste from print_debug_macinfo_section
into print_debug_macro_section where the nculist isn't actually used.

Not much to add, so pushed as is.

Thanks,

Mark

> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  src/readelf.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/src/readelf.c b/src/readelf.c
> index f09c5c9b..76ca65f5 100644
> --- a/src/readelf.c
> +++ b/src/readelf.c
> @@ -10247,7 +10247,6 @@ print_debug_macro_section (Dwfl_Module *dwflmod __attribute__ ((unused)),
>    Dwarf_Off ncu = 0;
>    size_t hsize;
>    struct mac_culist *culist = NULL;
> -  size_t nculist = 0;
>    while (dwarf_nextcu (dbg, offset = ncu, &ncu, &hsize, NULL, NULL, NULL) == 0)
>      {
>        Dwarf_Die cudie;
> @@ -10268,7 +10267,6 @@ print_debug_macro_section (Dwfl_Module *dwflmod __attribute__ ((unused)),
>        newp->files = NULL;
>        newp->next = culist;
>        culist = newp;
> -      ++nculist;
>      }
>  
>    const unsigned char *readp = (const unsigned char *) data->d_buf;

Re: [PATCH RFC 06/11] Initialize reglocs for VMCOREINFO

[Mark Wielaard]

Hi Ilya,

On Mon, 2023-02-06 at 23:25 +0100, Ilya Leoshkevich via Elfutils-devel
wrote:
> MSan complains:
> 
>     Uninitialized value was created by an allocation of 'reglocs' in the stack frame
>        #0 0x562d35c686f0 in handle_core_note elfutils/src/readelf.c:12674:3
>        #const Ebl_Register_Location *reglocs;
>     ==1006199==WARNING: MemorySanitizer: use-of-uninitialized-value
>        #0 0x562d35c68a2a in handle_core_note elfutils/src/readelf.c:12692:11
>        #colno = handle_core_registers (ebl, ebl->elf, desc + regs_offset,
>        #                               reglocs, nregloc);
> 
> Strictly speaking, this is not a problem, because nregloc == 0, but for
> other note types we initialize it anyway, so do it here as well.

Yeah, this is something valgrind wouldn't complain about since it
doesn't see passing of an undefined value as "use". But I think msan is
technically correct that passing an indeterminate value to a function
provokes undefined behavior. Also it is of course more consistent with
the rest of the code which does initialize reglocs even when nregloc is
zero.

Pushed as is.

Thanks,

Mark


> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  backends/linux-core-note.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/backends/linux-core-note.c b/backends/linux-core-note.c
> index 9faae4c3..238ec16d 100644
> --- a/backends/linux-core-note.c
> +++ b/backends/linux-core-note.c
> @@ -239,6 +239,7 @@ EBLHOOK(core_note) (const GElf_Nhdr *nhdr, const char *name,
>  	return 0;
>        *regs_offset = 0;
>        *nregloc = 0;
> +      *reglocs = NULL;
>        *nitems = 1;
>        *items = vmcoreinfo_items;
>        return 1;

Re: [PATCH RFC 10/11] configure: Add --disable-demangle

[Mark Wielaard]

Hi Ilya,

On Mon, 2023-02-06 at 23:25 +0100, Ilya Leoshkevich via Elfutils-devel
wrote:
> __cxa_demangle is normally implemented in the C++ runtime library,
> instrumenting which for MSan is a hassle. Add a knob for disabling it.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  configure.ac | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 7dc9be63..6a5c38af 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -466,11 +466,17 @@ CFLAGS="$CFLAGS -D_GNU_SOURCE"
>  AC_FUNC_STRERROR_R()
>  CFLAGS="$old_CFLAGS"
>  
> +AC_ARG_ENABLE([demangler],
> +AS_HELP_STRING([--disable-demangle],
> +               [Disable libstdc++ demangle support]))

Typo [--disable-demangler] (missing r).
Also we want to enable demangler support by default if the enable
option isn't given, so you need to add [], [enable_demangler=yes] at
the end.

AC_ARG_ENABLE (feature, help-string, [action-if-given], [action-if-not-
given])

So the action-if-not-given is to enable the feature.

Note that otherwise the summary at the end will have nothing for:

    libstdc++ demangle support         : 

Should be either yes or no, even when --enable-demangler or --disable-
demangler isn't given.

Cheers,

Mark

> +AS_IF([test "x$enable_demangler" == xyes],
>  AC_CHECK_LIB([stdc++], [__cxa_demangle], [dnl
>  AC_DEFINE([USE_DEMANGLE], [1], [Defined if demangling is enabled])])
>  AM_CONDITIONAL(DEMANGLE, test "x$ac_cv_lib_stdcpp___cxa_demangle" = "xyes")
>  AS_IF([test "x$ac_cv_lib_stdcpp___cxa_demangle" = "xyes"],
> -      [enable_demangler=yes],[enable_demangler=no])
> +      [enable_demangler=yes],[enable_demangler=no]),
> +AM_CONDITIONAL(DEMANGLE, false))
>  
>  AC_ARG_ENABLE([textrelcheck],
>  AS_HELP_STRING([--disable-textrelcheck],

Re: [PATCH RFC 07/11] addr2line: Do not test demangling in run-addr2line-i-test.sh

[Mark Wielaard]

Hi Ilya,

On Mon, 2023-02-06 at 23:25 +0100, Ilya Leoshkevich via Elfutils-devel
wrote:
> There is run-addr2line-i-demangle-test.sh for that.

Well spotted. Pushed as is.

Thanks,

Mark

> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  tests/run-addr2line-i-test.sh | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/run-addr2line-i-test.sh b/tests/run-addr2line-i-test.sh
> index 4f63e487..e7b89083 100755
> --- a/tests/run-addr2line-i-test.sh
> +++ b/tests/run-addr2line-i-test.sh
> @@ -254,13 +254,13 @@ EOF
>  
>  testfiles testfile-inlines-lto
>  
> -testrun_compare ${abs_top_builddir}/src/addr2line --pretty -fiC -e testfile-inlines-lto 0x1118 0x1137 <<\EOF
> -foobar(int) at /tmp/x.cpp:4:14
> - (inlined by) foo(int) at /tmp/x.cpp:22:16
> - (inlined by) fu(int) at /tmp/x.cpp:27:13
> -fubar(int) at /tmp/x.cpp:10:14
> - (inlined by) bar(int) at /tmp/x.cpp:16:15
> - (inlined by) fu(int) at /tmp/x.cpp:27:24
> +testrun_compare ${abs_top_builddir}/src/addr2line --pretty -fi -e testfile-inlines-lto 0x1118 0x1137 <<\EOF
> +_Z6foobari at /tmp/x.cpp:4:14
> + (inlined by) _Z3fooi at /tmp/x.cpp:22:16
> + (inlined by) _Z2fui at /tmp/x.cpp:27:13
> +_Z5fubari at /tmp/x.cpp:10:14
> + (inlined by) _Z3bari at /tmp/x.cpp:16:15
> + (inlined by) _Z2fui at /tmp/x.cpp:27:24
>  EOF
>  
>  exit 0

Re: [PATCH RFC 03/11] printversion: Fix unused variable

[Mark Wielaard]

Hi Ilya,

On Wed, 2023-02-08 at 13:22 +0100, Ilya Leoshkevich wrote:
> If I build:
> 
> const char *const apba__ __asm ("argp_program_bug_address") \
> __attribute__ ((used)) = "foobarbaz";
> 
> with C and C++, the difference is going to be:
> 
> @@ -1,6 +1,5 @@
>  	.file	"1.c"
>  	.text
> -	.globl	argp_program_bug_address
>  	.section	.rodata.str1.1,"aMS",@progbits,1
>  .LC0:
>  	.string	"foobarbaz"
> 
> This must have to do with C and C++ standards treating const
> differently [1]. The solution is to add extern:
> 
> --- a/lib/printversion.h
> +++ b/lib/printversion.h
> @@ -44,6 +44,7 @@ void print_version (FILE *stream, struct argp_state
> *state);
>    void (*const apvh) (FILE *, struct argp_state *) \
>     __asm ("argp_program_version_hook")
>  #define ARGP_PROGRAM_BUG_ADDRESS_DEF \
> +  extern const char *const apba__; \
>    const char *const apba__ __asm ("argp_program_bug_address") \
>    __attribute__ ((used))
> 
> I can include this in v2 if it works for you.
> 
> [1]
> https://stackoverflow.com/questions/8908071/const-correctness-in-c-vs-c

O nice, that explains it. But then in that case I don't think you need
the __attribute__ ((used)) anymore.

Also as a nitpick the multiline define could be just a single line if
you declare the extern on its own in printversion.h.

And it would be nice to also cleanup apvh/argp_program_version_hook so
it too works with c++, so we can remove the hack in debuginfod.cxx.

Does the following work for you?

diff --git a/lib/printversion.h b/lib/printversion.h
index a9e059ff..bc9ca7ae 100644
--- a/lib/printversion.h
+++ b/lib/printversion.h
@@ -40,9 +40,11 @@ void print_version (FILE *stream, struct argp_state *state);
    variables as non-const (which is correct in general).  But we can
    do better, it is not going to change.  So we want to move them into
    the .rodata section.  Define macros to do the trick.  */
+extern void (*const apvh) (FILE *, struct argp_state *);
 #define ARGP_PROGRAM_VERSION_HOOK_DEF \
   void (*const apvh) (FILE *, struct argp_state *) \
    __asm ("argp_program_version_hook")
+extern const char *const apba__;
 #define ARGP_PROGRAM_BUG_ADDRESS_DEF \
   const char *const apba__ __asm ("argp_program_bug_address")
 
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index 4271acf4..99b1f2b9 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -348,7 +348,7 @@ static const char DEBUGINFOD_SQLITE_CLEANUP_DDL[] =
 
 
 /* Name and version of program.  */
-/* ARGP_PROGRAM_VERSION_HOOK_DEF = print_version; */ // not this simple for C++
+ARGP_PROGRAM_VERSION_HOOK_DEF = print_version;
 
 /* Bug report address.  */
 ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT;
@@ -4171,7 +4171,6 @@ main (int argc, char *argv[])
 
   /* Parse and process arguments.  */
   int remaining;
-  argp_program_version_hook = print_version; // this works
   (void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER, &remaining, NULL);
   if (remaining != argc)
       error (EXIT_FAILURE, 0,

Thanks,

Mark

Re: [PATCH RFC 03/11] printversion: Fix unused variable

[Ilya Leoshkevich]

On Thu, 2023-02-09 at 15:04 +0100, Mark Wielaard wrote:
> Hi Ilya,
> 
> On Wed, 2023-02-08 at 13:22 +0100, Ilya Leoshkevich wrote:
> > If I build:
> > 
> > const char *const apba__ __asm ("argp_program_bug_address") \
> > __attribute__ ((used)) = "foobarbaz";
> > 
> > with C and C++, the difference is going to be:
> > 
> > @@ -1,6 +1,5 @@
> >         .file   "1.c"
> >         .text
> > -       .globl  argp_program_bug_address
> >         .section        .rodata.str1.1,"aMS",@progbits,1
> >  .LC0:
> >         .string "foobarbaz"
> > 
> > This must have to do with C and C++ standards treating const
> > differently [1]. The solution is to add extern:
> > 
> > --- a/lib/printversion.h
> > +++ b/lib/printversion.h
> > @@ -44,6 +44,7 @@ void print_version (FILE *stream, struct
> > argp_state
> > *state);
> >    void (*const apvh) (FILE *, struct argp_state *) \
> >     __asm ("argp_program_version_hook")
> >  #define ARGP_PROGRAM_BUG_ADDRESS_DEF \
> > +  extern const char *const apba__; \
> >    const char *const apba__ __asm ("argp_program_bug_address") \
> >    __attribute__ ((used))
> > 
> > I can include this in v2 if it works for you.
> > 
> > [1]
> > https://stackoverflow.com/questions/8908071/const-correctness-in-c-vs-c
> 
> O nice, that explains it. But then in that case I don't think you
> need
> the __attribute__ ((used)) anymore.
> 
> Also as a nitpick the multiline define could be just a single line if
> you declare the extern on its own in printversion.h.
> 
> And it would be nice to also cleanup apvh/argp_program_version_hook
> so
> it too works with c++, so we can remove the hack in debuginfod.cxx.
> 
> Does the following work for you?
> 
> diff --git a/lib/printversion.h b/lib/printversion.h
> index a9e059ff..bc9ca7ae 100644
> --- a/lib/printversion.h
> +++ b/lib/printversion.h
> @@ -40,9 +40,11 @@ void print_version (FILE *stream, struct
> argp_state *state);
>     variables as non-const (which is correct in general).  But we can
>     do better, it is not going to change.  So we want to move them
> into
>     the .rodata section.  Define macros to do the trick.  */
> +extern void (*const apvh) (FILE *, struct argp_state *);
>  #define ARGP_PROGRAM_VERSION_HOOK_DEF \
>    void (*const apvh) (FILE *, struct argp_state *) \
>     __asm ("argp_program_version_hook")
> +extern const char *const apba__;
>  #define ARGP_PROGRAM_BUG_ADDRESS_DEF \
>    const char *const apba__ __asm ("argp_program_bug_address")
>  
> diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
> index 4271acf4..99b1f2b9 100644
> --- a/debuginfod/debuginfod.cxx
> +++ b/debuginfod/debuginfod.cxx
> @@ -348,7 +348,7 @@ static const char DEBUGINFOD_SQLITE_CLEANUP_DDL[]
> =
>  
>  
>  /* Name and version of program.  */
> -/* ARGP_PROGRAM_VERSION_HOOK_DEF = print_version; */ // not this
> simple for C++
> +ARGP_PROGRAM_VERSION_HOOK_DEF = print_version;
>  
>  /* Bug report address.  */
>  ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT;
> @@ -4171,7 +4171,6 @@ main (int argc, char *argv[])
>  
>    /* Parse and process arguments.  */
>    int remaining;
> -  argp_program_version_hook = print_version; // this works
>    (void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER, &remaining,
> NULL);
>    if (remaining != argc)
>        error (EXIT_FAILURE, 0,
> 
> Thanks,
> 
> Mark

This works for me, I will add this to v3. Thanks!