Page MenuHomePhabricator

Add functionality for rewriting symbols
AbandonedPublic

Authored by fjricci on Jul 12 2016, 7:44 PM.

Details

Reviewers
clayborg
Summary

Clang supports symbol rewrites via the -frewrite-map-file flag,
this patch adds complementary functionality in lldb.

Re-written symbols are required when, for example, two versions of the
same library are linked to the same binary, and the user needs to
differentiate the symbols.

The SymbolRewriter implemented in this patch will use a rewrite map
provided via the 'target symbols rewrite' command to lookup the original
symbol names instead of the names re-written by clang.

Diff Detail

Event Timeline

fjricci updated this revision to Diff 63772.Jul 12 2016, 7:44 PM
fjricci retitled this revision from to Add functionality for rewriting symbols.
fjricci updated this object.
fjricci added reviewers: clayborg, lattner.
fjricci added subscribers: sas, lldb-commits.
labath added a subscriber: labath.Jul 13 2016, 3:11 AM

Mostly just comments about the test from me.

include/lldb/Core/Module.h
240

The const, as specified here is useless. const SP & avoids a copy, shared_ptr<const SymbolRewriter> ensures the rewriter cannot be modified. You probably want one of those.

packages/Python/lldbsuite/test/lang/c/symbol_rewriter/TestSymbolRewriter.py
21

If these compilers don't even support the features, go with skip instead of xfail.

29

This is not going to work for remote targets on different architectures. If you don't need debug info, could you just avoid generating it in the first place (-g0 ?). Maybe then you would be able to get this working on windows as well.
If it doesn't work, then we should wire up this call to go through the Makefile, as it already knows how to find the right toolchain for cross-compilation.

fjricci marked 3 inline comments as done.Jul 13 2016, 9:27 AM
fjricci added inline comments.
packages/Python/lldbsuite/test/lang/c/symbol_rewriter/TestSymbolRewriter.py
29

Unfortunately, the '-g0' override will only work for dwarf tests. However, based on TestWithLimitDebugInfo, it looks like we can skip the test for non-dwarf symbols (@skipIf(debug_info=no_match(["dwarf"]))). I think this is probably the cleanest way to go.

fjricci updated this revision to Diff 63816.Jul 13 2016, 9:41 AM
fjricci marked an inline comment as done.

Fix const SP and update unit test

Will now only run the unit test as a dwarf test (with -g0),
but since we don't want to test the debug data anyway,
this shouldn't be an issue, and avoids the use of strip.

clayborg requested changes to this revision.Jul 13 2016, 11:22 AM
clayborg edited edge metadata.

If you can't get this right when debug info is present then I would rather not have this patch. We enabled debug info on your test and it will call the putchar() in the current library so this test will fail and I question the viability of the usefulness of this feature.

Also if you can get that fixed, should the rewrite map actually be associated with a target? Shouldn't the rewrite map just be associated with a lldb_private::Module instead? It doesn't seem like you would want this at the target level. And the command should be "target modules rewrite add --clang-rewrite-file rewrite.map". It is ok to store the rewrite map in the target, but there should probably be a map of module to SymbolRewriter in the target.

In general if we add a member variable to ModuleList that is a SymbolRewriterSP, then many of the target->GetImages().FindXXX calls don't need a new parameter.

Also see inlined comments.

If this can't coexist with debug info I would rather not have this patch.

include/lldb/Core/ModuleList.h
272

We should add a member variable to ModuleList:

lldb::SymbolRewriterSP m_rewriter_sp;

Then have target place the lldb::SymbolRewriterSP into it's module list. Then this extra parameter shouldn't be needed.

284

Ditto above comment

408

Ditto above comment

include/lldb/Target/Target.h
1139–1149

Why are we making a lldb::SymbolRewriterSP here? This should return an empty lldb::SymbolRewriterSP if no one has called the "target symbols rewrite" function. Any callers should check for an empty shared pointer before using it.

1626

any shared pointers should have a _sp suffix.

source/API/SBTarget.cpp
1805–1811

This isn't needed if you allow ModuleList to have a "lldb::SymbolRewriterSP m_rewriter_sp;" member variable. Remove this.

1837

This isn't needed if you allow ModuleList to have a "lldb::SymbolRewriterSP m_rewriter_sp;" member variable. Remove this.

2390–2394

This isn't needed if you allow ModuleList to have a "lldb::SymbolRewriterSP m_rewriter_sp;" member variable. Remove this.

source/Breakpoint/BreakpointResolverName.cpp
237

You might need to ask the target to get the rewriter for a specific module:

lldb::SymbolRewriterSP rewriter = context.target_sp ? context.target_sp->GetSymbolRewriterForModule(context.module_sp) : nullptr;

It depends on if the rewrite maps should be associated with a given module, or if they truly are global and apply to all modules?

Also add a "_sp" suffix to any variables that are shared pointers.

source/Commands/CommandObjectSource.cpp
428–434

This isn't needed if you allow ModuleList to have a "lldb::SymbolRewriterSP m_rewriter_sp;" member variable. Remove this.

This revision now requires changes to proceed.Jul 13 2016, 11:22 AM

Adding a feature to the debugger that doesn't work with debug info seems like a bad idea.

This approach seems fragile to me. I'm supposed to remember that some build system set some rewriter file that I then have to supply manually to get the view of symbols to match reality? We really try to avoid having to write notes on little pieces of paper and typing them back in to get debugging to work...

This should get recorded somewhere in the module that uses it, and then the module reader code would be able to find and apply it automatically. If this is passed to the compiler, it should get written into the DWARF. But it sounds more like it gets passed to then linker. So maybe we can stick this map file in a load command, or symbol that the symbol file reader can fetch.

You might need a command to give search paths for these remap files in case you move the around, but that's all that the user should need to do by hand.

Adrian Prantl suggested that we modify clang to update the debug info for the function. In the test case that was attached, the DWARF currently has a "putchar" function which should get a linkage name attached to it with "__my_putchar". Then the debug info can be correct and we won't need any modifications of the rewrite file stuff.

sas added a comment.Jul 13 2016, 1:31 PM

@jingham, @clayborg, this is indeed a bit fragile as having to specify your rewrite maps manually when debugging is very error-prone. I have a patch that fetches the path to the rewrite map from the debug info, I'm waiting for this one to go in to upload that other diff (trying to make changes smaller so the review is easier). The way I do this is by looking at the flags that were passed to the compiler, and get the path to the rewrite map from there automatically, which then requires no user interaction for this to work.

@clayborg: As you saw when running the test with debug info enabled, we might end up calling the non-rewritten putchar(), which is due to the compiler emitting debug symbols with the non-rewritten name. The -g0 option is just a workaround until we can fix that.

I suppose Adrian Prantl's idea of modifying the emitted DWARF to add a linkage name attached to the function would work. Does that mean we would only add an entry for the rewritten symbol when lldb parses the DWARF, and ignore the non-rewritten function name?

In D22294#483213, @sas wrote:

@jingham, @clayborg, this is indeed a bit fragile as having to specify your rewrite maps manually when debugging is very error-prone. I have a patch that fetches the path to the rewrite map from the debug info, I'm waiting for this one to go in to upload that other diff (trying to make changes smaller so the review is easier). The way I do this is by looking at the flags that were passed to the compiler, and get the path to the rewrite map from there automatically, which then requires no user interaction for this to work.

If we have the compiler modify the DWARF by adding a linkage name to the appropriate DW_TAG_subproggram, then we won't need any rewrite file stuff at all.

@clayborg: As you saw when running the test with debug info enabled, we might end up calling the non-rewritten putchar(), which is due to the compiler emitting debug symbols with the non-rewritten name. The -g0 option is just a workaround until we can fix that.

I suppose Adrian Prantl's idea of modifying the emitted DWARF to add a linkage name attached to the function would work. Does that mean we would only add an entry for the rewritten symbol when lldb parses the DWARF, and ignore the non-rewritten function name?

Basically the DWARF currently looks like:

0x0000002e:     DW_TAG_subprogram [2] *
                 DW_AT_low_pc( 0x0000000000000000 )
                 DW_AT_high_pc( 0x000000000000000e )
                 DW_AT_frame_base( rbp )
                 DW_AT_name( "putchar" )
                 DW_AT_decl_file( "/Volumes/work/gclayton/Desktop/symbol_rewriter/main.c" )
                 DW_AT_decl_line( 3 )
                 DW_AT_prototyped( 0x01 )
                 DW_AT_type( {0x0000007a} ( int ) )
                 DW_AT_external( 0x01 )

And we would modify it to emit this:

0x0000002e:     DW_TAG_subprogram [2] *
                 DW_AT_low_pc( 0x0000000000000000 )
                 DW_AT_high_pc( 0x000000000000000e )
                 DW_AT_frame_base( rbp )
                 DW_AT_name( "putchar" )
                 DW_AT_linkage_name( "__my_putchar" ) <<< Added by compiler
                 DW_AT_decl_file( "/Volumes/work/gclayton/Desktop/symbol_rewriter/main.c" )
                 DW_AT_decl_line( 3 )
                 DW_AT_prototyped( 0x01 )
                 DW_AT_type( {0x0000007a} ( int ) )
                 DW_AT_external( 0x01 )

If we do this, then we don't need any modifications for the rewrite stuff at all?

sas added a comment.Jul 13 2016, 2:06 PM
In D22294#483213, @sas wrote:

@jingham, @clayborg, this is indeed a bit fragile as having to specify your rewrite maps manually when debugging is very error-prone. I have a patch that fetches the path to the rewrite map from the debug info, I'm waiting for this one to go in to upload that other diff (trying to make changes smaller so the review is easier). The way I do this is by looking at the flags that were passed to the compiler, and get the path to the rewrite map from there automatically, which then requires no user interaction for this to work.

If we have the compiler modify the DWARF by adding a linkage name to the appropriate DW_TAG_subproggram, then we won't need any rewrite file stuff at all.

@clayborg: As you saw when running the test with debug info enabled, we might end up calling the non-rewritten putchar(), which is due to the compiler emitting debug symbols with the non-rewritten name. The -g0 option is just a workaround until we can fix that.

I suppose Adrian Prantl's idea of modifying the emitted DWARF to add a linkage name attached to the function would work. Does that mean we would only add an entry for the rewritten symbol when lldb parses the DWARF, and ignore the non-rewritten function name?

Basically the DWARF currently looks like:

0x0000002e:     DW_TAG_subprogram [2] *
                 DW_AT_low_pc( 0x0000000000000000 )
                 DW_AT_high_pc( 0x000000000000000e )
                 DW_AT_frame_base( rbp )
                 DW_AT_name( "putchar" )
                 DW_AT_decl_file( "/Volumes/work/gclayton/Desktop/symbol_rewriter/main.c" )
                 DW_AT_decl_line( 3 )
                 DW_AT_prototyped( 0x01 )
                 DW_AT_type( {0x0000007a} ( int ) )
                 DW_AT_external( 0x01 )

And we would modify it to emit this:

0x0000002e:     DW_TAG_subprogram [2] *
                 DW_AT_low_pc( 0x0000000000000000 )
                 DW_AT_high_pc( 0x000000000000000e )
                 DW_AT_frame_base( rbp )
                 DW_AT_name( "putchar" )
                 DW_AT_linkage_name( "__my_putchar" ) <<< Added by compiler
                 DW_AT_decl_file( "/Volumes/work/gclayton/Desktop/symbol_rewriter/main.c" )
                 DW_AT_decl_line( 3 )
                 DW_AT_prototyped( 0x01 )
                 DW_AT_type( {0x0000007a} ( int ) )
                 DW_AT_external( 0x01 )

If we do this, then we don't need any modifications for the rewrite stuff at all?

I think some understanding of the rewrites from the debugger is still required because the user will still enter commands like call putchar('a') and expect the putchar() they wrote to be called. That putchar() they expect is now called _my_putchar() and unless we rewrite the symbol they typed, they'll end up calling the non-rewritten putchar().

One thing to note is that adding the DW_AT_linkage_name entry to DW_TAG_subprogram will fix the bug that we are working around in the unit tests with -g0, so that is required regardless.

I think some understanding of the rewrites from the debugger is still required because the user will still enter commands like call putchar('a') and expect the putchar() they wrote to be called. That putchar() they expect is now called _my_putchar() and unless we rewrite the symbol they typed, they'll end up calling the non-rewritten putchar().

No, the function lookup will find both "putchar" and "_my_putchar" will be found in your debug info and map to an address that matches the symbol for "_my_putchar". So no rewrite stuff will be needed.

One thing to note is that adding the DW_AT_linkage_name entry to DW_TAG_subprogram will fix the bug that we are working around in the unit tests with -g0, so that is required regardless.

So the way LLDB resolves functions is the function lookup happens for "putchar". If multiple results are found, then we prefer the one from the current executable, which means we will use the local "putchar". If we find multiple and none are in the current shared library we will note that there are duplicates and the expression will fail.

So I vote we fix the DWARF and be done.

Note that during function lookup, we can find both "putchar" and "__my_putchar" in the debug info, so you will be able to call both.

sas added a comment.Jul 13 2016, 2:55 PM

Note that during function lookup, we can find both "putchar" and "__my_putchar" in the debug info, so you will be able to call both.

Correct, unless as you pointed out both symbols are in libraries, and not in the main executable.

Our experience using this feature of clang has been that both symbols being in libraries is the rule rather than the exception.

See r221548 in llvm which introduces the symbol rewriter pass. This functionality is used for things like interposing of libraries, etc, or experiments where two versions of the same library are running alongside each other. This means that in the general use case, you have both the non-rewritten as well as the rewritten library loaded in the process.

FWIW, the current version of lldb is already able to call _my_putchar, given that this is the name present in the Mach-O symbol table.

We always resolve duplicate symbols to the "closest" one to the current frame. So if there are two versions of putchar and one is in the library containing the current frame, we should find that one. If we aren't getting that right, that's a bug. So the correct symbol should get called in cases where it makes sense. But if you are in main, and you have two libraries with rewritten putchar's it isn't clear which one we should call, and I think it isn't all that unexpected that we would just raise an error. If the duplicate symbols are both exported, but there's other linker magic to pick one over the other in main, then we'd have to know about that to get the right symbol. But I think that's orthogonal to this discussion.

Jim

In D22294#483311, @sas wrote:

Note that during function lookup, we can find both "putchar" and "__my_putchar" in the debug info, so you will be able to call both.

Correct, unless as you pointed out both symbols are in libraries, and not in the main executable.

Our experience using this feature of clang has been that both symbols being in libraries is the rule rather than the exception.

See r221548 in llvm which introduces the symbol rewriter pass. This functionality is used for things like interposing of libraries, etc, or experiments where two versions of the same library are running alongside each other. This means that in the general use case, you have both the non-rewritten as well as the rewritten library loaded in the process.

FWIW, the current version of lldb is already able to call _my_putchar, given that this is the name present in the Mach-O symbol table.

And adding the rewriting stuff does nothing to help these duplicate symbols. If you are not stopped in a.out, and you try to call putchar, your expression will fail with duplicate symbols even with your rewriting stuff. So there is no need to add it. Just fix the DWARF.

lattner resigned from this revision.Jul 13 2016, 10:05 PM
lattner removed a reviewer: lattner.
fjricci abandoned this revision.Mar 29 2018, 7:55 AM