Re: Fix build and tests on non-Linux (Hurd)

[Pino Toscano <toscano dot pino at tiscali dot it>]

Hi,

In data sabato 27 giugno 2015 15:30:00, Mark Wielaard ha scritto:
> The only problem is that we cannot guarantee not to break things
> since in general elfutils targets the GNU/Linux platform. I really
> need to setup a buildbot so that people can provide slaves to
> automatically tests their preferred platform doesn't break.

That sounds like a good idea.

In any case, elfutils is part of Debian, and thus it gets built also
on the two current non-Linux ports, GNU/kFreeBSD and GNU/Hurd.

> > * Build
> > 
> > With the provided commits, most of sources (including tests) build
> > again.
> > 
> > The remaining issue is in src/strings.c, the unconditional usage of
> > MAP_POPULATE. which is Linux-only. A local workaround of defining it
> > to 0 if not already defined seemed to not cause further issues.
> 
> Yeah, it is slightly ugly, but it really is only a hint. So defining
> it to zero if it isn't defined aleady seems fine. This is done on the
> portable branch. Which I want to get rid of. So lets just do this on
> master.

Ok -- commit attached.

> > Let's analyze the failing tests:
> > | FAIL: run-native-test.sh
> > | ========================
> > | 
> > | SRCDIR/tests/allregs: dwfl_module_register_names: no error
> > 
> > allregs relies on the kernel modules handling, which is
> > non-functional since there are no "kernel modules" on the Hurd
> > (microkernel with userspace servers).
> 
> I don't think that is true. It seems run-native-tests.sh tests against
> user space with -e (for an executable file test) and -p (for a
> running process test), but not against kernel modules (-k or -K).

Ah, I see. Then what I wrote below apply here too, i.e. that no
filenames are currently provided in Hurd's procfs for /proc/$pid/maps
files.

> > I get a clearer error message about this with:
> > 
> > diff --git a/tests/allregs.c b/tests/allregs.c
> > index 901d4e8..d3d459e 100644
> > --- a/tests/allregs.c
> > +++ b/tests/allregs.c
> > @@ -158,6 +158,8 @@ main (int argc, char **argv)
> > 
> >    Dwfl_Module *mod = NULL;
> >    if (dwfl_getmodules (dwfl, &first_module, &mod, 0) < 0)
> >      error (EXIT_FAILURE, 0, "dwfl_getmodules: %s", dwfl_errmsg (-1));
> > +  if (mod == NULL)
> > +    error (EXIT_FAILURE, 0, "dwfl_getmodules: module not found");
> > 
> >    if (remaining == argc)
> >      {
> 
> Better to see first if dwfl_errno () == 0. It it is non-zero we do
> want to print the actual error reported.

Do you mean if dwfl_getmodules fails (i.e. in the existing error()
message), or before the check I proposed?

Also, would be an option to skip the test if no modules are available?
Or that is considered a mandatory condition?

> > Also, if I comment out the usage of allregs from run-native-test.sh,
> > the test passes.
> > 
> > | FAIL: dwfl-bug-fd-leak
> > | ======================
> > | 
> > | (empty)
> > 
> > This test actually crashes on Hurd, as the dwfl_addrmodule() at
> > dwfl-bug-fd-leak.c:68 returns null. This is most probably due to our
> > procfs emulation not showing yet filenames in /proc/$pid/maps files.
> > At least the patch below avoids the segfault of the test, still
> > leaving the failure:
> > 
> > diff --git a/tests/dwfl-bug-fd-leak.c b/tests/dwfl-bug-fd-leak.c
> > index 170a61a..f992b7a 100644
> > --- a/tests/dwfl-bug-fd-leak.c
> > +++ b/tests/dwfl-bug-fd-leak.c
> > @@ -65,7 +65,10 @@ elfutils_open (pid_t pid, Dwarf_Addr address)
> >      }
> >    else
> >      {
> > -      Elf *elf = dwfl_module_getelf (dwfl_addrmodule (dwfl, address), &bias);
> > +      Dwfl_Module *module = dwfl_addrmodule (dwfl, address);
> > +      if (module == NULL)
> > +       error (2, 0, "dwfl_addrmodule: no module available for 0x%llx", address);
> > +      Elf *elf = dwfl_module_getelf (module, &bias);
> >        if (elf == NULL)
> >         error (2, 0, "dwfl_module_getelf: %s", dwfl_errmsg (-1));
> >      }
> 
> That looks like the correct check to me.

OK -- commit attached.

> > | FAIL: run-deleted.sh
> > | ====================
> > | 
> > | SRCDIR/src/stack: dwfl_linux_proc_attach pid 1491: Function not
> > 
> > implemented
> > 
> > This is because dwfl_linux_proc_attach is implemented only on Linux,
> > returning ENOSYS everywhere else. I made the test skipped with the
> > following:
> > 
> > diff --git a/tests/backtrace-subr.sh b/tests/backtrace-subr.sh
> > index 790b4f4..bb306c0 100644
> > --- a/tests/backtrace-subr.sh
> > +++ b/tests/backtrace-subr.sh
> > @@ -74,6 +74,10 @@ check_unsupported()
> > 
> >      echo >&2 $testname: arch not supported
> >      exit 77
> >    
> >    fi
> > 
> > +  if grep -q -E ': dwfl_linux_proc_attach pid ([[:digit:]]+):
> > Function not implemented$' $err; then
> > +    echo >&2 $testname: OS not supported
> > +    exit 77
> > +  fi
> > 
> >  }
> >  
> >  check_native_unsupported()
> > 
> > but I'm not sure that would be the correct way (especially that it
> > relies on the string representation of ENOSYS).
> 
> It isn't really correct to do it that way, but we do already depend on
> the string representation of some error messages in some other tests.
> Lets just use it here too.

OK -- commit attached, done only in run-deleted.sh itself now (more
logic).

> Better would of course be to implement the support. Doesn't the hurd
> support ptrace?

There is a basic implementation, possibly not fully working though.
I'd avoid it for now, maybe in the future...

Thanks,
-- 
Pino Toscano

>From 66626ca55c3e458cdeaae4c401b9ce76b03963d9 Mon Sep 17 00:00:00 2001
From: Pino Toscano <toscano.pino@tiscali.it>
Date: Sat, 27 Jun 2015 18:07:01 +0200
Subject: [PATCH] strings: Define MAP_POPULATE if not defined already

Currently it is available on Linux only, and it is more an hint.

Signed-off-by: Pino Toscano <toscano.pino@tiscali.it>
---
 src/ChangeLog | 4 ++++
 src/strings.c | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/src/ChangeLog b/src/ChangeLog
index 7d5e001..208db86 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,7 @@
+2015-06-27  Pino Toscano  <toscano.pino@tiscali.it>
+
+	* src/strings.c: Define MAP_POPULATE if not defined already.
+
 2015-06-18  Mark Wielaard  <mjw@redhat.com>
 
 	* findtextrel.c (process_file): Free segments after use.
diff --git a/src/strings.c b/src/strings.c
index 88a3c2f..397ce42 100644
--- a/src/strings.c
+++ b/src/strings.c
@@ -43,6 +43,10 @@
 
 #include <system.h>
 
+#ifndef MAP_POPULATE
+# define MAP_POPULATE 0
+#endif
+
 
 /* Prototypes of local functions.  */
 static int read_fd (int fd, const char *fname, off64_t fdlen);
-- 
2.1.4

>From 2ab77cd4c148c9938fa581901bba7e1b2036046c Mon Sep 17 00:00:00 2001
From: Pino Toscano <toscano.pino@tiscali.it>
Date: Sat, 27 Jun 2015 18:33:37 +0200
Subject: [PATCH] tests: dwfl-bug-fd-leak: Guard against null module addresses

Do not crash if there is no module for the given address.

Signed-off-by: Pino Toscano <toscano.pino@tiscali.it>
---
 tests/ChangeLog          | 5 +++++
 tests/dwfl-bug-fd-leak.c | 5 ++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/tests/ChangeLog b/tests/ChangeLog
index 3461168..3e567b3 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,8 @@
+2015-06-27  Pino Toscano  <toscano.pino@tiscali.it>
+
+	* tests/dwfl-bug-fd-leak.c (elfutils_open): Check for null results of
+	dwfl_addrmodule.
+
 2015-06-26  Pino Toscano  <toscano.pino@tiscali.it>
 
 	* tests/vdsosyms.c [!__linux__] (main): Mark argv as unused.
diff --git a/tests/dwfl-bug-fd-leak.c b/tests/dwfl-bug-fd-leak.c
index 170a61a..f992b7a 100644
--- a/tests/dwfl-bug-fd-leak.c
+++ b/tests/dwfl-bug-fd-leak.c
@@ -65,7 +65,10 @@ elfutils_open (pid_t pid, Dwarf_Addr address)
     }
   else
     {
-      Elf *elf = dwfl_module_getelf (dwfl_addrmodule (dwfl, address), &bias);
+      Dwfl_Module *module = dwfl_addrmodule (dwfl, address);
+      if (module == NULL)
+	error (2, 0, "dwfl_addrmodule: no module available for 0x%llx", address);
+      Elf *elf = dwfl_module_getelf (module, &bias);
       if (elf == NULL)
 	error (2, 0, "dwfl_module_getelf: %s", dwfl_errmsg (-1));
     }
-- 
2.1.4

>From a3bfff0239cd8cf34adab5cc77c40fe45ad4da52 Mon Sep 17 00:00:00 2001
From: Pino Toscano <toscano.pino@tiscali.it>
Date: Sat, 27 Jun 2015 19:23:01 +0200
Subject: [PATCH] tests: skip run-deleted.sh when dwfl_linux_proc_attach is not
 implemented

If the current OS does not implement dwfl_linux_proc_attach (which
currently only Linux does) then skip this test, as "stack" uses that
API for attaching to a running process.

Signed-off-by: Pino Toscano <toscano.pino@tiscali.it>
---
 tests/ChangeLog      | 5 +++++
 tests/run-deleted.sh | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/tests/ChangeLog b/tests/ChangeLog
index 3e567b3..90b9a0a 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,5 +1,10 @@
 2015-06-27  Pino Toscano  <toscano.pino@tiscali.it>
 
+	* tests/run-deleted.sh: Skip when detecting a not implemented
+	dwfl_linux_proc_attach.
+
+2015-06-27  Pino Toscano  <toscano.pino@tiscali.it>
+
 	* tests/dwfl-bug-fd-leak.c (elfutils_open): Check for null results of
 	dwfl_addrmodule.
 
diff --git a/tests/run-deleted.sh b/tests/run-deleted.sh
index 2b5a9a8..0f64762 100755
--- a/tests/run-deleted.sh
+++ b/tests/run-deleted.sh
@@ -38,6 +38,10 @@ cat bt bt.err
 kill -9 $pid
 wait
 check_native_unsupported bt.err deleted
+if grep -q -E ': dwfl_linux_proc_attach pid ([[:digit:]]+): Function not implemented$' bt.err; then
+  echo >&2 deleted: OS not supported
+  exit 77
+fi
 # For PPC64 we need access to the OPD table which we get through the shdrs
 # (see backends/ppc64_init.c) but for the deleted-lib we only have phdrs.
 # So we don't have the name of the function. But since we should find
-- 
2.1.4

Attachment: signature.asc
Description: PGP signature