On Wed, Mar 28, 2012 at 02:54:49PM -0700, Roland McGrath wrote:
> On Wed, Mar 28, 2012 at 01:36:26PM -0700, Roland McGrath wrote:
> > As mentioned before, if we merge and finish up the relocate branch
> > then the problem goes away.
>
> But we currently don't have an estimate for when this will happen.
IIRC it was pretty well ready when I left it. I just merged the trunk
into the branch. It still builds except for strip.c where you copied
some of the libdwfl/relocate.c code. That needs to get the
corresponding updates to use ebl_reloc_simple_types instead of
ebl_reloc_simple_type. Do you want to make those fixes on the branch?
Thanks for that. I'll take a look.
I think the main barrier was wanting some more consideration on the
new
API additions (look at 'git diff master..roland/relocate -- libdw.h')
before we enshrined them in the permanent libdw ABI. So one option is
just to decide we think they are good enough as they are.
OK. I'll start a new review thread for that. I admit I didn't know it
was already that close.
Another option is to do an intermediate version of the branch with
all
the internals changes, but without making any new interfaces public.
Then libdwfl-based uses like systemtap's get the (presumed, measurable)
performance benefits of reloc-on-demand and the compressed-section
problem goes away for libdwfl. But we don't commit to any new APIs or
ABIs.
I can try it against some systemtap testcases to see how it goes.
> Until we do finish that I like to not apply relocations to
compressed
> sections, since we know that just doesn't work (and the corruption might
> not even be detected if we do). That is really all the patch does.
I have several concerns here. Even if we did just that, then I thought
the patch looked ugly enough that it can probably be done in a way
somewhat cleaner than that. But that's just the trivial concern.
Since I thought the patch was as clean as I could come up with please
do let me know what you thought was the ugly part. I might have to
adjust my "beautiful code" filter :) The patch really was minimal
IMHO:
diff --git a/libdwfl/relocate.c b/libdwfl/relocate.c
index 95206f4..effef44 100644
--- a/libdwfl/relocate.c
+++ b/libdwfl/relocate.c
@@ -319,6 +319,12 @@ relocate_section (Dwfl_Module *mod, Elf *relocated, const GElf_Ehdr
*ehdr,
if (tdata == NULL)
return DWFL_E_LIBELF;
+ if (tname[0] == '.' && tname[1] == 'z' &&
tdata->d_size >= 4 + 8
+ && memcmp (tdata->d_buf, "ZLIB", 4) == 0)
+ /* Compressed section can only be relocated after decompression,
+ don't touch it or we will corrupt it. */
+ return DWFL_E_NOERROR;
+
/* Apply one relocation. Returns true for any invalid data. */
Dwfl_Error relocate (GElf_Addr offset, const GElf_Sxword *addend,
int rtype, int symndx)
It concerns me more that we would be papering over our lack of
thorough
support for .zdebug_* sections while making it appear that we might
handle them. Giving such a false impression might be worse than an
"obviously broken" state.
OK, I was more concerned with the possibility of silently corrupting
the sections libdwfl prepared.
The support I added in commit 725aad5d was pretty minimal and I
surely
overlooked multiple subtleties at the time. (We weren't near a release
at the time, and I probably figured we'd have more close attention
before the next release than we actually had in the year--exactly one
year, it so happens--that we had from then until 0.153.) The libdwfl
interaction for ET_REL that's been noticed here is just one. Another
that pops to mind now is that ebl_debugscn_p doesn't match .zdebug_*
sections. What effects does that have?
Some strip variants would get confused, I can write some tests for that.
Funnily relocate () has a debug flag which checks ebl_debugscn_p and doesn't
relocate if the target isn't a ebl_debugscn_p (). But we never use that
flag.
Cheers,
Mark