[Lldb-commits] [PATCH] D22294: Add functionality for rewriting symbols

[Greg Clayton via lldb-commits]

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