Re: [PATCH] Don't look for kernel version if not running on linux
You can get uname() on Windows through gnulib, but at the cost at linking to additional windows DLLs. It calls gethostname and for that we need to link against ws2_32.dll. That's an unreasonable dependency for getting something we cannot use anyway. I suggest we just set errno to ENOTSUP then. I should clarify this a bit. We only use uname() to obtain the kernel version and we only use the kernel version to search for directories containing kernel modules. The only operating system where this makes sense is linux as we cannot handle anything but linux kernel modules there. Therefore there is no point in retrieving the kernel version on any other OS. We might still be able to load linux kernel modules from some directory on a different OS, but the automatic detection of that directory can be skipped. Ulf
Re: [PATCH] Make __attribute__ conditional in all installed headers
On Thu, 2017-04-20 at 15:55 +0200, Ulf Hermann wrote: > __attribute__ is a GNU extension. If we want to link against the > libraries using a different compiler, it needs to be disabled. It was > already disabled in libdw.h, and this patch extends this to the other > headers. We move the defines to libelf.h as that is included in all > the others. Looks like a nice cleanup in general. My only concern was that libelf.h is a "more" public header than libdw.h and so the macros could in theory clash with other code. But I couldn't find any other code that uses them except elfutils code itself. And if there was any then we already exported them through libdw.h anyway. It does add another explicit dependency between elfutils libdw and elfutils libelf, but given that libdw already does have some dependencies on elfutils libelf internals I don't think that matters. Applied to master. Thanks, Mark
Re: [PATCH] Use F_GETFD rather than F_GETFL to check validity of file descriptor
On Thu, 2017-04-20 at 15:58 +0200, Ulf Hermann wrote: > F_GETFD is both cheaper and easier to port, and otherwise has the same > effect here. At least for the specific case of checking for failure with errno == EBADF it is identical. Applied to master. Thanks, Mark
Re: [PATCH] Avoid double-including config.h
On Thu, 2017-04-20 at 16:31 +0200, Ulf Hermann wrote: > config.h doesn't have include guards, so including it twice is bad. We > deal with this by checking for PACKAGE_NAME, but only in some places. > Once we start using gnulib, we will need to include config.h before any > gnulib-generated headers. This is problematic if we include it > transitively through our own private headers. > > In order to set a clear rule about inclusion of config.h, it is now > included in every .c file as first header, but not in any header. This > will definitely avoid double-inclusion and satisfy the condition that it > has to be included before gnulib headers. It comes at the price of > adding some redundancy, but there is no clean way to avoid this. It seems a clear rule that is easy to follow. The only exceptions are the crc32.c file which gets included itself and the linux-kernel-modules.c which needs the ugly BAD_FTS check before config.h (which your patch both handles). Maybe we can cleanup that last one once we integrate gnulib (which I believe has a good/64-off_t fts.h already). Applied to master. Thanks, Mark
Re: [PATCH] Avoid double-including config.h
Maybe we can cleanup that last one once we integrate gnulib (which I believe has a good/64-off_t fts.h already). gnulib is among the bad ones. Or, at least we detect it as bad. Ulf
Re: [PATCH 5/5] Add frame pointer unwinding for aarch64
On Thu, 2017-04-27 at 10:31 +0200, Ulf Hermann wrote: > > Maybe something like the attached patch? > > Well that's actually the original patch (as opposed to V2) with relaxed > test conditions. You can write that a bit nicer by setting the new PC > directly after retrieving LR and returning early if it doesn't work. See > "[PATCH 2/3] Add frame pointer unwinding as fallback on arm" from > February 16th. That's the original algorithm; for aarch64 I just added a > few defines and included arm_unwind.c. OK, I made that cleanup and rebased the mjw/fp-unwind branch to master. All patches on that branch are now pushed so for 0.169 we should have a frame pointer unwinder backtrace fallback for at least x86_64, i686 and aarch64. Thanks for all your work and sorry this took a while to land. Cheers, Mark
Re: [PATCH] Don't look for kernel version if not running on linux
On Tue, May 02, 2017 at 12:17:24PM +0200, Ulf Hermann wrote:
> > You can get uname() on Windows through gnulib, but at the cost at
> > linking to additional windows DLLs. It calls gethostname and for that
> > we need to link against ws2_32.dll. That's an unreasonable dependency
> > for getting something we cannot use anyway. I suggest we just set
> > errno to ENOTSUP then.
>
> I should clarify this a bit. We only use uname() to obtain the kernel
> version and we only use the kernel version to search for directories
> containing kernel modules. The only operating system where this makes sense
> is linux as we cannot handle anything but linux kernel modules there.
> Therefore there is no point in retrieving the kernel version on any other
> OS.
Agreed. So committed as attached.
Thanks,
Mark
>From e88787f9cd2af5be00aa6f53320cf85f7c08f1f2 Mon Sep 17 00:00:00 2001
From: Ulf Hermann
Date: Thu, 20 Apr 2017 16:08:48 +0200
Subject: [PATCH] Don't look for kernel version if not running on linux
We don't want to use it, even if it exists.
Signed-off-by: Ulf Hermann
Signed-off-by: Mark Wielaard
---
libdwfl/ChangeLog | 6 ++
libdwfl/linux-kernel-modules.c | 7 +++
2 files changed, 13 insertions(+)
diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index 1fc9da6..859b2ff 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,4 +1,10 @@
2017-04-20 Ulf Hermann
+ Mark Wielaard
+
+ * linux-kernel-modules.c: Always return NULL from kernel_release() on
+ non-linux systems and set errno to ENOTSUP.
+
+2017-04-20 Ulf Hermann
* libdwflP.h: Don't include config.h.
* argp-std.c: Include config.h.
diff --git a/libdwfl/linux-kernel-modules.c b/libdwfl/linux-kernel-modules.c
index 893110a..9d0fef2 100644
--- a/libdwfl/linux-kernel-modules.c
+++ b/libdwfl/linux-kernel-modules.c
@@ -156,11 +156,18 @@ try_kernel_name (Dwfl *dwfl, char **fname, bool try_debug)
static inline const char *
kernel_release (void)
{
+#ifdef __linux__
/* Cache the `uname -r` string we'll use. */
static struct utsname utsname;
if (utsname.release[0] == '\0' && uname (&utsname) != 0)
return NULL;
return utsname.release;
+#else
+ /* Used for finding the running linux kernel, which isn't supported
+ on non-linux kernel systems. */
+ errno = ENOTSUP;
+ return NULL;
+#endif
}
static int
--
2.9.3
Re: [PATCH] Make elf section sorting more deterministic
On Fri, Apr 28, 2017 at 12:35:26PM +0200, Ulf Hermann wrote: > On 04/27/2017 09:41 PM, Mark Wielaard wrote: > > On Thu, Apr 20, 2017 at 04:54:26PM +0200, Ulf Hermann wrote: > >> At least one test (dwfl-addr-sect) depends on the order of elf sections > >> with equal addresses. This is not guaranteed by the code. Compare also > >> by end address and name to tell entries apart. > > > > O, interesting find. If the start addresses match the order depends on > > the specific qsort algorithm. So you need a real tie breaker. > > > > I think it is simpler and more predictable if we just take the section > > number into account. It seem to have the added benefit that it provide > > the same ordering as before with the glibc qsort, so no testcases need > > to be adjusted. Does the following work for you? > > > > diff --git a/libdwfl/derelocate.c b/libdwfl/derelocate.c > > index 439a24e..0d10672 100644 > > --- a/libdwfl/derelocate.c > > +++ b/libdwfl/derelocate.c > > @@ -63,7 +63,10 @@ compare_secrefs (const void *a, const void *b) > >if ((*p1)->start > (*p2)->start) > > return 1; > > > > - return 0; > > + /* Same start address, then just compare which section came first. */ > > + size_t n1 = elf_ndxscn ((*p1)->scn); > > + size_t n2 = elf_ndxscn ((*p2)->scn); > > + return n1 - n2; > > I would inline the whole thing to > > return elf_ndxscn (p1->scn) - elf_ndxscn (p2->scn); > > There is no point in forcing the compiler to keep the intermediate > numbers as (signed) size_t. OK. > Also, I would still keep the check for p1->end and p2->end before this. > If we have a section of size 0, and another one of size > 0 starting at > the same place, we want them to be sorted by end address. The zero-sized > section should be squeezed in before the one that actually has a size, > not after it. Aha. I see that we treat the end special in find_section () /* Consider the limit of a section to be inside it, unless it's inside the next one. A section limit address can appear in line records. */ if (*addr == sections->refs[idx].end && idx + 1 < sections->count && *addr == sections->refs[idx + 1].start) ++idx; So we do indeed want the zero sized section to be the first. How about the attached? Cheers, Mark >From d7e1e08f68afd6c3363231e3100fa0124a2d5cb6 Mon Sep 17 00:00:00 2001 From: Ulf Hermann Date: Thu, 20 Apr 2017 16:54:26 +0200 Subject: [PATCH] Make elf section sorting more deterministic At least one test (dwfl-addr-sect) depends on the order of elf sections with equal addresses. This is not guaranteed by the code. Compare also by end address and section index to tell entries apart. Signed-off-by: Ulf Hermann Signed-off-by: Mark Wielaard --- libdwfl/ChangeLog| 6 ++ libdwfl/derelocate.c | 8 +++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog index 859b2ff..9bce6b1 100644 --- a/libdwfl/ChangeLog +++ b/libdwfl/ChangeLog @@ -1,6 +1,12 @@ 2017-04-20 Ulf Hermann Mark Wielaard + * derelocate.c (compare_secrefs): Compare by end address and then by + section number if addresses are equal. + +2017-04-20 Ulf Hermann + Mark Wielaard + * linux-kernel-modules.c: Always return NULL from kernel_release() on non-linux systems and set errno to ENOTSUP. diff --git a/libdwfl/derelocate.c b/libdwfl/derelocate.c index e5c3e12..2f80b20 100644 --- a/libdwfl/derelocate.c +++ b/libdwfl/derelocate.c @@ -67,7 +67,13 @@ compare_secrefs (const void *a, const void *b) if ((*p1)->start > (*p2)->start) return 1; - return 0; + if ((*p1)->end < (*p2)->end) +return -1; + if ((*p1)->end > (*p2)->end) +return 1; + + /* Same start/end, then just compare which section came first. */ + return elf_ndxscn ((*p1)->scn) - elf_ndxscn ((*p2)->scn); } static int -- 2.9.3
