Re: [PATCH] Use separate files for strip outputs

2019-01-24 Thread Mark Wielaard
On Wed, 2019-01-23 at 23:19 +0100, Mark Wielaard wrote:
> As you say in your commit message this exposes that the
> run-strip-test-many.sh actually fails. Indeed, even without
> you patch you can see src/strip: illformed file 'testfile'
> in the run-strip-test-many.sh.log (but without actually failing
> the test...
> 
> I am still investgating why/how it is failing.
> Will apply your patch afterwards.

Turned out to it was a onliner:

diff --git a/src/strip.c b/src/strip.c
index 1518073..a73009d 100644
--- a/src/strip.c
+++ b/src/strip.c
@@ -1944,7 +1944,7 @@ handle_elf (int fd, Elf *elf, const char *prefix,
const char *fname,
  INTERNAL_ERROR (fname);
 
if (sym->st_shndx == SHN_UNDEF
-   || (sym->st_shndx >= shnum
+   || (sym->st_shndx >= SHN_LORESERVE
&& sym->st_shndx != SHN_XINDEX))
  {
/* This is no section index, leave it alone

This caused special section numbers like SHN_ABS and SHN_COMMON to not
be recognized as special, so they got "renumbered". Which was obviously
bogus.

Fixing this did expose that elflint is very strict with regards to the
.shstrtab name and type (with strip -g we will not move the old
.shstrtab section and so change its type to SHT_NOBITS). This might be
a bit overzealous of elflint. But for now I just changed the
addsections test program to change the old name and name the new one
.shstrtab.

With that your cleaned up test case passes.

Pushed both to master.

Thanks,

Mark
From 4540ea98ce3314c84273f5c9cbb8b774f1463dc4 Mon Sep 17 00:00:00 2001
From: Mark Wielaard 
Date: Thu, 24 Jan 2019 16:00:49 +0100
Subject: [PATCH] strip: Fix check test for SHN_XINDEX symbol.

The check for whether a symbol used the extended section table was
wrong causing the run-strip-test-many.sh testcase to declare the
testfile was an illformed file.

Fixing this exposed a strict elfutils check for the '.shstrtab'
section having this exact name and a SHT_STRTAB type. This might
be a little too strict, but easily worked around by changing the
name of the "old" shstrtab section in the addsections program.

Signed-off-by: Mark Wielaard 
---
 src/ChangeLog   |  4 
 src/strip.c |  2 +-
 tests/ChangeLog |  6 ++
 tests/addsections.c | 37 -
 4 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/src/ChangeLog b/src/ChangeLog
index 0ea106c..0544fce 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,7 @@
+2019-01-24  Mark Wielaard  
+
+	* strip.c (handle_elf): Fix check test for SHN_XINDEX symbol.
+
 2019-01-22  Mark Wielaard  
 
 	* readelf.c (print_debug_line_section): Check we are not at end of
diff --git a/src/strip.c b/src/strip.c
index 1518073..a73009d 100644
--- a/src/strip.c
+++ b/src/strip.c
@@ -1944,7 +1944,7 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 		  INTERNAL_ERROR (fname);
 
 		if (sym->st_shndx == SHN_UNDEF
-			|| (sym->st_shndx >= shnum
+			|| (sym->st_shndx >= SHN_LORESERVE
 			&& sym->st_shndx != SHN_XINDEX))
 		  {
 			/* This is no section index, leave it alone
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 8c9e780..2dd8026 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,9 @@
+2019-01-24  Mark Wielaard  
+
+	* addsections.c (add_sections): Change the name of the old shstrtab
+	section to ".old_shstrtab" and give the old shstrtab name to the
+	new shstrtab section.
+
 2019-01-09  Ulf Hermann 
 
 	* run-readelf-compressed.sh: Skip if USE_BZIP2 not found.
diff --git a/tests/addsections.c b/tests/addsections.c
index 391c5b4..cc8d065 100644
--- a/tests/addsections.c
+++ b/tests/addsections.c
@@ -92,7 +92,7 @@ add_sections (const char *name, size_t nr, int use_mmap)
 
   /* We will add a new shstrtab section with two new names at the end.
  Just get the current shstrtab table and add two entries '.extra'
- and '.new_shstrtab' at the end of the table, so all existing indexes
+ and '.old_shstrtab' at the end of the table, so all existing indexes
  are still valid.  */
   size_t shstrndx;
   if (elf_getshdrstrndx (elf, &shstrndx) < 0)
@@ -115,7 +115,7 @@ add_sections (const char *name, size_t nr, int use_mmap)
 }
   size_t new_shstrtab_size = (shstrtab_data->d_size
 			  + strlen (".extra") + 1
-			  + strlen (".new_shstrtab") + 1);
+			  + strlen (".old_shstrtab") + 1);
   void *new_shstrtab_buf = malloc (new_shstrtab_size);
   if (new_shstrtab_buf == NULL)
 {
@@ -124,9 +124,30 @@ add_sections (const char *name, size_t nr, int use_mmap)
 }
   memcpy (new_shstrtab_buf, shstrtab_data->d_buf, shstrtab_data->d_size);
   size_t extra_idx = shstrtab_data->d_size;
-  size_t new_shstrtab_idx = extra_idx + strlen (".extra") + 1;
+  size_t old_shstrtab_idx = extra_idx + strlen (".extra") + 1;
   strcpy (new_shstrtab_buf + extra_idx, ".extra");
-  strcpy (new_shstrtab_buf + new_sh

Re: [PATCH] configure: Add new --enable-install-elfh option.

2019-01-24 Thread Mark Wielaard
On Fri, 2019-01-18 at 14:03 +, Ulf Hermann wrote:
> I think you should also adapt tests/Makefile.am to use our own elf.h
> in 
> this case. See https://codereview.qt-project.org/#/c/187812/25 for
> my 
> solution to this.

Yes, it should indeed.
I used a slightly different solution though.
It relies on the default include flags already including the srcdirs.
Does that work for your use case too? (See revised patch attached.)

I looked at the features.h tweak also in your patch.
But I am not comfortable including something like that.
It feels like it needs a different test (whether we are actually on a
POSIX system or not). And I am not sure it really should define uid_t,
gid_t, mode_t, and pid_t. Those normally don't come from features.h
(they would come from sys/types.h).

Cheers,

Mark
From 86f9481187bccb78b2533674bb905a0de1a03abf Mon Sep 17 00:00:00 2001
From: Mark Wielaard 
Date: Fri, 18 Jan 2019 14:18:22 +0100
Subject: [PATCH] configure: Add new --enable-install-elfh option.

We explicitly test (with system-elf-libelf) that our include headers
work with the system elf.h header. But it might be helpful to install
the elf.h file for a private install. Our elf.h header really is just
a copy of the latest glibc elf.h. But it might be newer and include
more constants than the system installed elf.h.

Add a new configure option --enable-install-elfh to install elf.h.
But warn when it is enabled for the default /usr or /usr/local prefix
because it might clash with the glibc/system elf.h header in that case.

Signed-off-by: Mark Wielaard 
---
 ChangeLog  |  4 
 configure.ac   | 12 
 libelf/ChangeLog   |  5 +
 libelf/Makefile.am | 13 ++---
 tests/ChangeLog|  5 +
 tests/Makefile.am  |  4 +++-
 6 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 45418a0..148ce77 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2019-01-18  Mark Wielaard  
+
+	* configure.ac: Add new --enable-install-elfh.
+
 2018-07-04  Ross Burton 
 
 	* configure.ac: Check for gawk.
diff --git a/configure.ac b/configure.ac
index b89b867..7d4e69d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -323,6 +323,11 @@ if test "$use_valgrind" = yes; then
 fi
 AM_CONDITIONAL(USE_VALGRIND, test "$use_valgrind" = yes)
 
+AC_ARG_ENABLE([install-elfh],
+AS_HELP_STRING([--enable-install-elfh],[install elf.h in include dir]),
+   [install_elfh=$enableval], [install_elfh=no])
+AM_CONDITIONAL(INSTALL_ELFH, test "$install_elfh" = yes)
+
 AM_CONDITIONAL(BUILD_STATIC, [dnl
 test "$use_gprof" = yes -o "$use_gcov" = yes])
 
@@ -658,6 +663,7 @@ AC_MSG_NOTICE([
 
   NOT RECOMMENDED FEATURES (should all be no)
 Experimental thread safety : ${use_locks}
+install elf.h  : ${install_elfh}
 
   OTHER FEATURES
 Deterministic archives by default  : ${default_ar_deterministic}
@@ -673,3 +679,9 @@ AC_MSG_NOTICE([
 use rpath in tests : ${tests_use_rpath}
 test biarch: ${utrace_cv_cc_biarch}
 ])
+
+if test "$install_elfh" = yes; then
+  if test "${prefix}" = "/usr/local" -o "${prefix}" = "/usr"; then
+AC_MSG_WARN([installing elf.h in ${includedir} might conflict with glibc/system elf.h])
+  fi
+fi
diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 5783f0c..b89e93f 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,3 +1,8 @@
+2019-01-18  Mark Wielaard  
+
+	* Makefile.am (INSTALL_ELFH): Add elf.h to include_HEADERS when
+	defined, otherwise (the default) add elf.h to noinst_HEADERS.
+
 2019-01-16  Mark Wielaard  
 
 	* note_xlate.h (elf_cvt_note): Check n_namesz and n_descsz don't
diff --git a/libelf/Makefile.am b/libelf/Makefile.am
index ddaeaa2..d5d63f7 100644
--- a/libelf/Makefile.am
+++ b/libelf/Makefile.am
@@ -39,6 +39,16 @@ noinst_LIBRARIES = libelf_pic.a
 noinst_PROGRAMS = $(noinst_LIBRARIES:_pic.a=.so)
 include_HEADERS = libelf.h gelf.h nlist.h
 
+noinst_HEADERS = abstract.h common.h exttypes.h gelf_xlate.h libelfP.h \
+		 version_xlate.h gnuhash_xlate.h note_xlate.h dl-hash.h \
+		 chdr_xlate.h
+
+if INSTALL_ELFH
+include_HEADERS += elf.h
+else
+noinst_HEADERS += elf.h
+endif
+
 pkginclude_HEADERS = elf-knowledge.h
 
 libelf_a_SOURCES = elf_version.c elf_hash.c elf_error.c elf_fill.c \
@@ -123,9 +133,6 @@ uninstall: uninstall-am
 	rm -f $(DESTDIR)$(libdir)/libelf.so.$(VERSION)
 	rm -f $(DESTDIR)$(libdir)/libelf.so
 
-noinst_HEADERS = elf.h abstract.h common.h exttypes.h gelf_xlate.h libelfP.h \
-		 version_xlate.h gnuhash_xlate.h note_xlate.h dl-hash.h \
-		 chdr_xlate.h
 EXTRA_DIST = libelf.map
 
 CLEANFILES += $(am_libelf_pic_a_OBJECTS) libelf.so.$(VERSION)
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 8c9e780..1a7bc81 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,8 @@
+2019-01-24  Mark Wielaard  
+
+	* Makefile.am (system_elf_libelf_test_CPPFLAGS): Guard by
+	!INSTALL_ELFH.
+
 2019-01-09  Ulf Hermann 
 
 	* run-readelf-compressed.sh: S