clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
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.
================
Comment at: include/lldb/Core/ModuleList.h:272
@@ -271,2 +271,3 @@
FindFunctions (const ConstString &name,
+ const lldb::SymbolRewriterSP &rewriter,
uint32_t name_type_mask,
----------------
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.
================
Comment at: include/lldb/Core/ModuleList.h:284
@@ -282,2 +283,3 @@
FindFunctionSymbols (const ConstString &name,
+ const lldb::SymbolRewriterSP &rewriter,
uint32_t name_type_mask,
----------------
Ditto above comment
================
Comment at: include/lldb/Core/ModuleList.h:408
@@ -405,2 +407,3 @@
FindSymbolsWithNameAndType (const ConstString &name,
+ const lldb::SymbolRewriterSP &rewriter,
lldb::SymbolType symbol_type,
----------------
Ditto above comment
================
Comment at: include/lldb/Target/Target.h:1139-1149
@@ -1137,2 +1138,13 @@
+
+ lldb::SymbolRewriterSP
+ GetSymbolRewriter ()
+ {
+ if (!m_symbol_rewriter)
+ {
+ m_symbol_rewriter = std::make_shared<SymbolRewriter>();
+ }
+
+ return m_symbol_rewriter;
+ }
//------------------------------------------------------------------
----------------
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.
================
Comment at: include/lldb/Target/Target.h:1626
@@ -1613,2 +1625,3 @@
ModuleList m_images; ///< The list of images for this
process (shared libraries and anything dynamically loaded).
+ lldb::SymbolRewriterSP m_symbol_rewriter; ///< Symbol rewriter for the
target.
SectionLoadHistory m_section_load_history;
----------------
any shared pointers should have a _sp suffix.
================
Comment at: source/API/SBTarget.cpp:1805-1811
@@ -1804,8 +1804,9 @@
const bool append = true;
target_sp->GetImages().FindFunctions (ConstString(name),
+
target_sp->GetSymbolRewriter(),
name_type_mask,
symbols_ok,
inlines_ok,
append,
*sb_sc_list);
}
----------------
This isn't needed if you allow ModuleList to have a "lldb::SymbolRewriterSP
m_rewriter_sp;" member variable. Remove this.
================
Comment at: source/API/SBTarget.cpp:1837
@@ -1835,3 +1836,3 @@
default:
- target_sp->GetImages().FindFunctions(ConstString(name),
eFunctionNameTypeAny, true, true, true, *sb_sc_list);
+ target_sp->GetImages().FindFunctions(ConstString(name),
target_sp->GetSymbolRewriter(), eFunctionNameTypeAny, true, true, true,
*sb_sc_list);
break;
----------------
This isn't needed if you allow ModuleList to have a "lldb::SymbolRewriterSP
m_rewriter_sp;" member variable. Remove this.
================
Comment at: source/API/SBTarget.cpp:2390-2394
@@ -2388,6 +2389,7 @@
bool append = true;
target_sp->GetImages().FindSymbolsWithNameAndType
(ConstString(name),
+
target_sp->GetSymbolRewriter(),
symbol_type,
*sb_sc_list,
append);
}
----------------
This isn't needed if you allow ModuleList to have a "lldb::SymbolRewriterSP
m_rewriter_sp;" member variable. Remove this.
================
Comment at: source/Breakpoint/BreakpointResolverName.cpp:237
@@ -235,2 +236,3 @@
const size_t start_func_idx = func_list.GetSize();
+ const lldb::SymbolRewriterSP &rewriter = context.target_sp
? context.target_sp->GetSymbolRewriter() : nullptr;
context.module_sp->FindFunctions(lookup.lookup_name,
----------------
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.
================
Comment at: source/Commands/CommandObjectSource.cpp:428-434
@@ -427,8 +427,9 @@
m_module_list : target->GetImages();
size_t num_matches = module_list.FindFunctions(name,
+
target->GetSymbolRewriter(),
eFunctionNameTypeAuto,
/*include_symbols=*/false,
/*include_inlines=*/true,
/*append=*/true,
sc_list_funcs);
if (!num_matches)
----------------
This isn't needed if you allow ModuleList to have a "lldb::SymbolRewriterSP
m_rewriter_sp;" member variable. Remove this.
http://reviews.llvm.org/D22294
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
