[PATCH] D42582: [lldb][PPC64] Fixed step-in stopping in the wrong line

[Leandro Lupori via Phabricator via llvm-commits]

luporl updated this revision to Diff 135303.
luporl added a comment.
Herald added subscribers: arichardson, emaste.

- Revert "Changed GetBytesToSkip comment to focus on the API"
- Revert "Fixed ArchitecturePPC64 initialization"
- Revert "Moved changes to new ArchitecturePPC64 plugin"
- Revert "Fixed step-in stopping in the wrong line in PPC64"
- Handling PPC64 local entry point with extra symbol
- Merge branch 'master' into fix-step-in2

This new change is an attempt to handle PPC64 global/local entry points with
trampoline symbols.

This "works" for step in, but it doesn't look good and I had to deviate enough
from the last suggestion from Jim in order to make it work (see item (3)
below). Or perhaps it was I that didn't understand it correctly.

The following are descriptions of what did not work:

1- Set the global entry point (GEP) as a trampoline to the local entry point

  (LEP)

Issues:

- Although the forwarding to the LEP works, LLDB doesn't know how to handle a "LEP", that is different than functions entered from the other trampoline symbols in LLDB.
- The address from the LEP won't match the debug information of where the function starts, which causes issues in step in logic and prologue size calculation.
- It also messes up with the disassembly of the function: instructions will start at a non-zero offset from the function symbol (because the function symbol is always equal to GEP address).

2- Set both entry points as trampolines to the prologue end

  (to be able to handle "skip past prologue" correctly)

Issues:

- Trampoline symbols don't work well with GetPrologueSize() code.
- To try to make this work without too much effort, it would be needed yet another symbol at prologue end, and custom code to find the prologue end.
- This would probably have other side effects, as with unwind assembly, for instance, that would get a function without prologue to process.

This is what worked (this change):

3- Setting the LEP as a trampoline (actually, as a local symbol) to prologue

  end
  
  To avoid the issues above, it looks like the best way is to avoid modifying
  the original function symbol (a.k.a. the GEP).
  Also note that step in already works when the function is entered by the
  global entry point. For most purposes, it's just like a regular function
  start address, that is already handled correctly by LLDB.

Issues:

- The LEP trampoline gets shadowed by the function symbol, which causes the DynamicLoader to not know that it's in its address by regular means. To work around this, the code needs to perform several steps to look for the LEP symbol, whenever it could be have been shadowed by the GEP, which can be expensive.
- Trampoline symbols are skipped from name lookup, so for the bullet above to work, the symbol type must be something else.

In the end, it seems to me that trying to handle PPC64 GEP/LEP with
trampolines causes more trouble than it solves.

IMHO, what we need is a clean way to check if a function has more than one
entry point, and to get its address. Then we could just make some small
changes to code parts that may need to consider more than one entry point,
such as the step in plan, that in this case would just need to perform an
additional check to see if the current address matches any known entry point.


https://reviews.llvm.org/D42582

Files:
  source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp


Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===================================================================
--- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -2377,6 +2377,34 @@
     bool symbol_size_valid =
         symbol.st_size != 0 || symbol.getType() != STT_FUNC;
 
+    // PPC64: create an extra symbol for functions with local entry points.
+    const llvm::Triple::ArchType llvm_arch = arch.GetMachine();
+    if (llvm_arch == llvm::Triple::ppc64 ||
+        llvm_arch == llvm::Triple::ppc64le) {
+      int64_t loffs = llvm::ELF::decodePPC64LocalEntryOffset(symbol.st_other);
+      if (loffs) {
+        Symbol lep(i + start_id,      // Symbol table index
+                   symbol_name,       // symbol name.
+                   is_mangled,        // is the symbol name mangled?
+                   eSymbolTypeLocal,  // Type of this symbol
+                                      // (trampoline types can't be looked up by
+                                      // name)
+                   false,             // Is this globally visible?
+                   false,             // Is this symbol debug info?
+                   true,              // Is this symbol a trampoline?
+                   true,              // Is this symbol artificial?
+                   symbol_section_sp, // Section in which this symbol
+                                      // is defined or null.
+                   symbol_value + loffs, // Offset in section or symbol value.
+                   0,                    // Size in bytes of this symbol.
+                   false,                // Size is valid
+                   false,                // Contains linker annotations?
+                   0);                   // Symbol flags.
+
+        symtab->AddSymbol(lep);
+      }
+    }
+
     Symbol dc_symbol(
         i + start_id, // ID is the original symbol table index.
         mangled,
Index: source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
===================================================================
--- source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
+++ source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
@@ -24,6 +24,7 @@
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/Platform.h"
 #include "lldb/Target/Process.h"
+#include "lldb/Target/RegisterContext.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Target/ThreadPlanRunToAddress.h"
@@ -460,6 +461,36 @@
   }
 }
 
+static bool HandlePPC64LocalEntryPoint(Thread &thread, bool stop, Symbol *sym,
+                                       ThreadPlanSP &thread_plan_sp) {
+  Target &target = thread.GetProcess()->GetTarget();
+  if (target.GetArchitecture().GetMachine() != llvm::Triple::ppc64le)
+    return false;
+
+  ConstString sym_name = sym->GetMangled().GetMangledName();
+  if (!sym_name)
+    return false;
+
+  lldb::addr_t current_address = thread.GetRegisterContext()->GetPC(0);
+  lldb::addr_t gep_addr = sym->GetLoadAddress(&target);
+  if (current_address == gep_addr)
+    return false;
+
+  const ModuleList &images = target.GetImages();
+  SymbolContextList target_symbols;
+  images.FindSymbolsWithNameAndType(sym_name, eSymbolTypeLocal, target_symbols);
+  if (target_symbols.GetSize() != 1)
+    return false;
+  Symbol *lep = target_symbols[0].symbol;
+
+  if (current_address != lep->GetLoadAddress(&target))
+    return false;
+
+  std::vector<lldb::addr_t> addrs = {gep_addr + sym->GetPrologueByteSize()};
+  thread_plan_sp.reset(new ThreadPlanRunToAddress(thread, addrs, stop));
+  return true;
+}
+
 ThreadPlanSP
 DynamicLoaderPOSIXDYLD::GetStepThroughTrampolinePlan(Thread &thread,
                                                      bool stop) {
@@ -469,6 +500,9 @@
   const SymbolContext &context = frame->GetSymbolContext(eSymbolContextSymbol);
   Symbol *sym = context.symbol;
 
+  if (HandlePPC64LocalEntryPoint(thread, stop, sym, thread_plan_sp))
+    return thread_plan_sp;
+
   if (sym == NULL || !sym->IsTrampoline())
     return thread_plan_sp;
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D42582.135303.patch
Type: text/x-patch
Size: 4142 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180221/1f2b0ad5/attachment.bin>