Hi,
dependent on all the patches listed in: Pending patches ping https://lists.fedorahosted.org/pipermail/elfutils-devel/2012-November/002742...
present in: jankratochvil/forunwind-baseaddr but this branch is not suitable for merge as it is unclear in which form get checked in the patches above. The branch is usable only for "git checkout", neither diff nor merge should be done on it.
Current dwfl_report_elf function has parameter "GElf_Addr base" but it means bias, not the base address. Some applications (such as Linux kernel perf) cannot easily find out the bias but they know the base address. I tried to implement it without this function but I could not.
Thanks, Jan
libdwfl/ 2012-11-13 Jan Kratochvil jan.kratochvil@redhat.com
* dwfl_report_elf.c (__libdwfl_report_elf): Add parameter base_is_bias. Set BASE_IS_BIAS to true for ET_EXEC and ET_CORE. Provide alternative setup of START and BIAS if !BASE_IS_BIAS. Set END from BIAS, not BASE. (dwfl_report_elf): Rename to ... (__libdwfl_report_elf_open): ... here. Add parameter base_is_bias, pass it to __libdwfl_report_elf. (dwfl_report_elf, dwfl_report_elf_baseaddr): New wrappers. * libdwfl.h (dwfl_report_elf_baseaddr): New declaration. * libdwflP.h (__libdwfl_report_elf): Add parameter base_is_bias. * offline.c (process_elf): Pass true as base_is_bias to __libdwfl_report_elf.
diff --git a/libdwfl/dwfl_report_elf.c b/libdwfl/dwfl_report_elf.c index d706170..7a6970e 100644 --- a/libdwfl/dwfl_report_elf.c +++ b/libdwfl/dwfl_report_elf.c @@ -41,7 +41,8 @@ Dwfl_Module * internal_function __libdwfl_report_elf (Dwfl *dwfl, const char *name, const char *file_name, - int fd, Elf *elf, GElf_Addr base, bool sanity) + int fd, Elf *elf, GElf_Addr base, bool base_is_bias, + bool sanity) { GElf_Ehdr ehdr_mem, *ehdr = gelf_getehdr (elf, &ehdr_mem); if (ehdr == NULL) @@ -166,6 +167,7 @@ __libdwfl_report_elf (Dwfl *dwfl, const char *name, const char *file_name, case ET_CORE: /* An assigned base address is meaningless for these. */ base = 0; + base_is_bias = true;
case ET_DYN: default:; @@ -181,11 +183,19 @@ __libdwfl_report_elf (Dwfl *dwfl, const char *name, const char *file_name, { vaddr = ph->p_vaddr & -ph->p_align; address_sync = ph->p_vaddr + ph->p_memsz; - start = base + vaddr; break; } } - bias = base; + if (base_is_bias) + { + start = base + vaddr; + bias = base; + } + else + { + start = base; + bias = base - vaddr; + }
for (size_t i = phnum; i-- > 0;) { @@ -195,7 +205,7 @@ __libdwfl_report_elf (Dwfl *dwfl, const char *name, const char *file_name, if (ph->p_type == PT_LOAD && ph->p_vaddr + ph->p_memsz > 0) { - end = base + (ph->p_vaddr + ph->p_memsz); + end = bias + (ph->p_vaddr + ph->p_memsz); break; } } @@ -246,8 +256,9 @@ __libdwfl_report_elf (Dwfl *dwfl, const char *name, const char *file_name, }
Dwfl_Module * -dwfl_report_elf (Dwfl *dwfl, const char *name, - const char *file_name, int fd, GElf_Addr base) +internal_function +__libdwfl_report_elf_open (Dwfl *dwfl, const char *name, const char *file_name, + int fd, GElf_Addr base, bool base_is_bias) { bool closefd = false; if (fd < 0) @@ -270,7 +281,7 @@ dwfl_report_elf (Dwfl *dwfl, const char *name, }
Dwfl_Module *mod = __libdwfl_report_elf (dwfl, name, file_name, - fd, elf, base, true); + fd, elf, base, base_is_bias, true); if (mod == NULL) { elf_end (elf); @@ -280,4 +291,19 @@ dwfl_report_elf (Dwfl *dwfl, const char *name,
return mod; } + +Dwfl_Module * +dwfl_report_elf (Dwfl *dwfl, const char *name, + const char *file_name, int fd, GElf_Addr base) +{ + return __libdwfl_report_elf_open (dwfl, name, file_name, fd, base, true); +} INTDEF (dwfl_report_elf) + +Dwfl_Module * +dwfl_report_elf_baseaddr (Dwfl *dwfl, const char *name, + const char *file_name, int fd, GElf_Addr base) +{ + return __libdwfl_report_elf_open (dwfl, name, file_name, fd, base, false); +} +INTDEF (dwfl_report_elf_baseaddr) diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h index 8498c0c..a2ab824 100644 --- a/libdwfl/libdwfl.h +++ b/libdwfl/libdwfl.h @@ -146,6 +146,13 @@ extern Dwfl_Module *dwfl_report_elf (Dwfl *dwfl, const char *name, const char *file_name, int fd, GElf_Addr base);
+/* See dwfl_report_elf except that BASE is the page-aligned absolute VMA + address where the ELF file should start. Any possible file prelinking of + the disk file is compensated. */ +extern Dwfl_Module *dwfl_report_elf_baseaddr (Dwfl *dwfl, const char *name, + const char *file_name, int fd, + GElf_Addr base); + /* Similar, but report the module for offline use. All ET_EXEC files being reported must be reported before any relocatable objects. If this is used, dwfl_report_module and dwfl_report_elf may not be diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h index ca8be2f..e5e638c 100644 --- a/libdwfl/libdwflP.h +++ b/libdwfl/libdwflP.h @@ -353,7 +353,8 @@ extern int __libdwfl_crc32_file (int fd, uint32_t *resp) attribute_hidden; Consumes ELF on success, not on failure. */ extern Dwfl_Module *__libdwfl_report_elf (Dwfl *dwfl, const char *name, const char *file_name, int fd, - Elf *elf, GElf_Addr base, bool sanity) + Elf *elf, GElf_Addr base, + bool base_is_bias, bool sanity) internal_function;
/* Meat of dwfl_report_offline. */ diff --git a/libdwfl/offline.c b/libdwfl/offline.c index 26a6bd6..28d2782 100644 --- a/libdwfl/offline.c +++ b/libdwfl/offline.c @@ -127,7 +127,8 @@ process_elf (Dwfl *dwfl, const char *name, const char *file_name, int fd, Elf *elf) { Dwfl_Module *mod = __libdwfl_report_elf (dwfl, name, file_name, fd, elf, - dwfl->offline_next_address, false); + dwfl->offline_next_address, true, + false); if (mod != NULL) { /* If this is an ET_EXEC file with fixed addresses, the address range
On Tue, 2012-11-13 at 21:30 +0100, Jan Kratochvil wrote:
Current dwfl_report_elf function has parameter "GElf_Addr base" but it means bias, not the base address.
If the documentation is wrong/misleading in libdwfl.h we really should also fix/clarify that.
I admit that I always get a little confused myself. If I got it right then the bias should refer to the offset from the p_vaddr of an ELF segment. And the base should refer the low address of the Dwfl_Module, which is fixed for the main ET_EXEC and ET_CORE at the lowest p_vaddr in the ELF file, but dynamic for ET_DYN (and the kernel).
Even if I got the above right I might still get confused about the meaning of the terms for ET_REL (there are no p_hdrs, so the terms don't really make sense there do they?). So please do correct me if I seem wrong/confused.
Some applications (such as Linux kernel perf) cannot easily find out the bias but they know the base address. I tried to implement it without this function but I could not.
Is using dwfl_report_elf () vs dwfl_report_elf_baseaddr () only different for ET_DYN files, or does it also change things for reporting other ELF files?
Thanks,
Mark
On Fri, 15 Mar 2013 17:13:13 +0100, Mark Wielaard wrote:
On Tue, 2012-11-13 at 21:30 +0100, Jan Kratochvil wrote:
Current dwfl_report_elf function has parameter "GElf_Addr base" but it means bias, not the base address.
If the documentation is wrong/misleading in libdwfl.h we really should also fix/clarify that.
The documentation is correct but I added there the prelink caveat.
I admit that I always get a little confused myself. If I got it right then the bias should refer to the offset from the p_vaddr of an ELF segment.
yes
And the base should refer the low address of the Dwfl_Module, which is fixed for the main ET_EXEC and ET_CORE at the lowest p_vaddr in the ELF file, but dynamic for ET_DYN (and the kernel).
yes
Even if I got the above right I might still get confused about the meaning of the terms for ET_REL (there are no p_hdrs, so the terms don't really make sense there do they?).
ET_REL does not make sense to map into memory... For real .o elfutils does whatever may be considered as user convenient. For Linux kernel ET_REL files in fact I do not know much, you definitely know more.
But as a summary the dwfl_report_elf <-> dwfl_report_elf_base difference is only for prelinked files and prelink affects (in VMA way) only ET_DYN files.
Some applications (such as Linux kernel perf) cannot easily find out the bias but they know the base address. I tried to implement it without this function but I could not.
Is using dwfl_report_elf () vs dwfl_report_elf_baseaddr () only different for ET_DYN files, or does it also change things for reporting other ELF files?
yes, only ET_DYN and only if they are prelinked. Written it therefore now in the comment.
Thanks, Jan
[ this updated version is not present in any GIT branch ]
commit 15ce3ffbdf8005e85891d994aa9e2d9578767eee Author: Jan Kratochvil jan.kratochvil@redhat.com Date: Sun Mar 17 14:07:27 2013 +0100
libdwfl/ * dwfl_report_elf.c (__libdwfl_report_elf): Add parameter base_is_bias. Set BASE_IS_BIAS to true for ET_EXEC and ET_CORE. Provide alternative setup of START and BIAS if !BASE_IS_BIAS. Set END from BIAS, not BASE. (dwfl_report_elf): Rename to ... (__libdwfl_report_elf_open): ... here. Add parameter base_is_bias, pass it to __libdwfl_report_elf. (dwfl_report_elf, dwfl_report_elf_baseaddr): New wrappers. * libdwfl.h (dwfl_report_elf_baseaddr): New declaration. * libdwflP.h (__libdwfl_report_elf): Add parameter base_is_bias. * offline.c (process_elf): Pass true as base_is_bias to __libdwfl_report_elf.
diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog index f55625c..4427a08 100644 --- a/libdwfl/ChangeLog +++ b/libdwfl/ChangeLog @@ -1,3 +1,17 @@ +2013-03-17 Jan Kratochvil jan.kratochvil@redhat.com + + * dwfl_report_elf.c (__libdwfl_report_elf): Add parameter base_is_bias. + Set BASE_IS_BIAS to true for ET_EXEC and ET_CORE. Provide alternative + setup of START and BIAS if !BASE_IS_BIAS. Set END from BIAS, not BASE. + (dwfl_report_elf): Rename to ... + (__libdwfl_report_elf_open): ... here. Add parameter base_is_bias, + pass it to __libdwfl_report_elf. + (dwfl_report_elf, dwfl_report_elf_baseaddr): New wrappers. + * libdwfl.h (dwfl_report_elf_baseaddr): New declaration. + * libdwflP.h (__libdwfl_report_elf): Add parameter base_is_bias. + * offline.c (process_elf): Pass true as base_is_bias to + __libdwfl_report_elf. + 2013-03-15 Jan Kratochvil jan.kratochvil@redhat.com
* dwfl_report_elf.c (__libdwfl_report_elf): Remove BASE aligning. diff --git a/libdwfl/dwfl_report_elf.c b/libdwfl/dwfl_report_elf.c index d706170..7a6970e 100644 --- a/libdwfl/dwfl_report_elf.c +++ b/libdwfl/dwfl_report_elf.c @@ -41,7 +41,8 @@ Dwfl_Module * internal_function __libdwfl_report_elf (Dwfl *dwfl, const char *name, const char *file_name, - int fd, Elf *elf, GElf_Addr base, bool sanity) + int fd, Elf *elf, GElf_Addr base, bool base_is_bias, + bool sanity) { GElf_Ehdr ehdr_mem, *ehdr = gelf_getehdr (elf, &ehdr_mem); if (ehdr == NULL) @@ -166,6 +167,7 @@ __libdwfl_report_elf (Dwfl *dwfl, const char *name, const char *file_name, case ET_CORE: /* An assigned base address is meaningless for these. */ base = 0; + base_is_bias = true;
case ET_DYN: default:; @@ -181,11 +183,19 @@ __libdwfl_report_elf (Dwfl *dwfl, const char *name, const char *file_name, { vaddr = ph->p_vaddr & -ph->p_align; address_sync = ph->p_vaddr + ph->p_memsz; - start = base + vaddr; break; } } - bias = base; + if (base_is_bias) + { + start = base + vaddr; + bias = base; + } + else + { + start = base; + bias = base - vaddr; + }
for (size_t i = phnum; i-- > 0;) { @@ -195,7 +205,7 @@ __libdwfl_report_elf (Dwfl *dwfl, const char *name, const char *file_name, if (ph->p_type == PT_LOAD && ph->p_vaddr + ph->p_memsz > 0) { - end = base + (ph->p_vaddr + ph->p_memsz); + end = bias + (ph->p_vaddr + ph->p_memsz); break; } } @@ -246,8 +256,9 @@ __libdwfl_report_elf (Dwfl *dwfl, const char *name, const char *file_name, }
Dwfl_Module * -dwfl_report_elf (Dwfl *dwfl, const char *name, - const char *file_name, int fd, GElf_Addr base) +internal_function +__libdwfl_report_elf_open (Dwfl *dwfl, const char *name, const char *file_name, + int fd, GElf_Addr base, bool base_is_bias) { bool closefd = false; if (fd < 0) @@ -270,7 +281,7 @@ dwfl_report_elf (Dwfl *dwfl, const char *name, }
Dwfl_Module *mod = __libdwfl_report_elf (dwfl, name, file_name, - fd, elf, base, true); + fd, elf, base, base_is_bias, true); if (mod == NULL) { elf_end (elf); @@ -280,4 +291,19 @@ dwfl_report_elf (Dwfl *dwfl, const char *name,
return mod; } + +Dwfl_Module * +dwfl_report_elf (Dwfl *dwfl, const char *name, + const char *file_name, int fd, GElf_Addr base) +{ + return __libdwfl_report_elf_open (dwfl, name, file_name, fd, base, true); +} INTDEF (dwfl_report_elf) + +Dwfl_Module * +dwfl_report_elf_baseaddr (Dwfl *dwfl, const char *name, + const char *file_name, int fd, GElf_Addr base) +{ + return __libdwfl_report_elf_open (dwfl, name, file_name, fd, base, false); +} +INTDEF (dwfl_report_elf_baseaddr) diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h index 000d582..7102bd8 100644 --- a/libdwfl/libdwfl.h +++ b/libdwfl/libdwfl.h @@ -139,13 +139,22 @@ extern Dwfl_Module *dwfl_report_module (Dwfl *dwfl, const char *name,
/* Report a module with start and end addresses computed from the ELF program headers in the given file, plus BASE. For an ET_REL file, - does a simple absolute section layout starting at BASE. + does a simple absolute section layout starting at BASE. For prelinked + ET_DYN files BASE is a difference between mapped VMA and disk file VMA. FD may be -1 to open FILE_NAME. On success, FD is consumed by the library, and the `find_elf' callback will not be used for this module. */ extern Dwfl_Module *dwfl_report_elf (Dwfl *dwfl, const char *name, const char *file_name, int fd, GElf_Addr base);
+/* See dwfl_report_elf except that BASE is the page-aligned absolute VMA + address where the ELF file should start. Any possible file prelinking of + the disk file is compensated. The difference happens only for prelinked + ET_DYN files - otherwise the function is equivalent to dwfl_report_elf. */ +extern Dwfl_Module *dwfl_report_elf_baseaddr (Dwfl *dwfl, const char *name, + const char *file_name, int fd, + GElf_Addr base); + /* Similar, but report the module for offline use. All ET_EXEC files being reported must be reported before any relocatable objects. If this is used, dwfl_report_module and dwfl_report_elf may not be diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h index 5aaa778..204444f 100644 --- a/libdwfl/libdwflP.h +++ b/libdwfl/libdwflP.h @@ -382,7 +382,8 @@ extern int __libdwfl_crc32_file (int fd, uint32_t *resp) attribute_hidden; Consumes ELF on success, not on failure. */ extern Dwfl_Module *__libdwfl_report_elf (Dwfl *dwfl, const char *name, const char *file_name, int fd, - Elf *elf, GElf_Addr base, bool sanity) + Elf *elf, GElf_Addr base, + bool base_is_bias, bool sanity) internal_function;
/* Meat of dwfl_report_offline. */ diff --git a/libdwfl/offline.c b/libdwfl/offline.c index 26a6bd6..28d2782 100644 --- a/libdwfl/offline.c +++ b/libdwfl/offline.c @@ -127,7 +127,8 @@ process_elf (Dwfl *dwfl, const char *name, const char *file_name, int fd, Elf *elf) { Dwfl_Module *mod = __libdwfl_report_elf (dwfl, name, file_name, fd, elf, - dwfl->offline_next_address, false); + dwfl->offline_next_address, true, + false); if (mod != NULL) { /* If this is an ET_EXEC file with fixed addresses, the address range
I haven't been tracking the details, but I am confused. I don't want to add another function to the API. What is the rationale for adding dwfl_report_elf_baseaddr?
Thanks, Roland
Hi Roland,
On Mon, 18 Mar 2013 20:35:53 +0100, Roland McGrath wrote:
I haven't been tracking the details, but I am confused. I don't want to add another function to the API. What is the rationale for adding dwfl_report_elf_baseaddr?
a later patch contains function report_module in a test application:
input: PC anywhere in a shared library of some PID uses: /proc/PID/maps to find out VMA where the file is mapped from output: ELF file mapped into Dwfl address space
If report_module used dwfl_report_elf then it would need to adjust the base VMA found in /proc/PID/maps by where the DSO file is currently prelinked to.
dwfl_report_elf's parameter base = "bias" = 'in-memory VMA' - 'on-disk VMA'
This would mean the application (containing report_module) would need to parse the ELF file to determine the current on-disk file prelink offset. I do not find it easy and it will even have poor performance opening and parsing the ELF file twice. For dwfl_report_elf it is just a new flag.
(It is irrelevant where the DSO was prelinked to when PID started.)
Thanks, Jan
The alternative question is who wants dwfl_report_elf with the current meaning of its third argument? It's used by link_map.c, but that could just call an internal routine instead.
I'd rather not have multiple public entrypoints for this. I don't think it's a problem to change the public API a bit, since it doesn't have many users. If we really need both modes, then I'm more inclined to have one function with more parameters than to have two functions.
Thanks, Roland
On Mon, Mar 18, 2013 at 01:25:25PM -0700, Roland McGrath wrote:
The alternative question is who wants dwfl_report_elf with the current meaning of its third argument? It's used by link_map.c, but that could just call an internal routine instead.
The current meaning of the third argument vs ET_DYN files is certainly confusing. At least to me. It wasn't till Jan explained it to me that I realized what really was happening. So having base always mean base does look like a good idea to me.
I haven't found any users of dwfl_report_elf outside the elfutils code base. But that doesn't mean there aren't any. Is there any way we can let the user signal they want the new semantics? Maybe have the module name argument have some prefix or suffix. Or is that even uglier?
Cheers,
Mark
I haven't found any users of dwfl_report_elf outside the elfutils code base. But that doesn't mean there aren't any. Is there any way we can let the user signal they want the new semantics? Maybe have the module name argument have some prefix or suffix. Or is that even uglier?
That is laughably ugly. You are such a joker, I'm sure you didn't mean it.
What I had in mind was something like making the function take two arguments, e.g. "GElf_Addr base, bool add_p_vaddr". So N, true would mean the old semantics. That has the benefit that the API changes in a compile-breaking way, so nobody will just recompile and not notice their program being broken if it was right before.
It's a bit ugly for a function interface, especially if nobody ever actually wants the old behavior. But keeping the same signature for different semantics means silent change on recompile, creating confusion (probably much later).
Of course, we also need not to break old binaries with a new DSO. But that is easy enough with symbol versioning.
Thanks, Roland
On Mon, 2013-03-18 at 14:09 -0700, Roland McGrath wrote:
I haven't found any users of dwfl_report_elf outside the elfutils code base. But that doesn't mean there aren't any. Is there any way we can let the user signal they want the new semantics? Maybe have the module name argument have some prefix or suffix. Or is that even uglier?
That is laughably ugly. You are such a joker, I'm sure you didn't mean it.
:) Yeah, lets pretend I didn't mean that.
What I had in mind was something like making the function take two arguments, e.g. "GElf_Addr base, bool add_p_vaddr". So N, true would mean the old semantics. That has the benefit that the API changes in a compile-breaking way, so nobody will just recompile and not notice their program being broken if it was right before.
It's a bit ugly for a function interface, especially if nobody ever actually wants the old behavior. But keeping the same signature for different semantics means silent change on recompile, creating confusion (probably much later).
OK, that is indeed nicer. A pity it breaks source compatibility. But you are right that people that do use this function now might not really know/want the semantics it currently has.
Of course, we also need not to break old binaries with a new DSO. But that is easy enough with symbol versioning.
If I understand the NEW_VERSION and COMPAT_VERSION macros (as used with dwfl_module_build_id) correctly then that indeed seems easy.
Thanks,
Mark
elfutils-devel@lists.fedorahosted.org