Avoid GEP when creating a breakpoint

[luporl]

Summary

On PPC64le architecture, a function may have two entry points:
Global Entry Point (GEP)
Local Entry Point (LEP)

When entered though GEP, it executes extra instructions, which, in most cases, adjusts the TOC pointer.
The LEP comes right after these extra instructions.

When called locally (through LEP), this code is not executed, so when creating a breakpoint with the function
name, these instructions must be avoided, using the LEP instead of the GEP.

[labath]

I don't know if this is the right way to fix this issue, but if it is, I think this function should be refactored/split/whatever somehow to avoid peppering it with "else need_adjust_break_address = true;" everywhere.

Also a test would be in order (looks like a "lldb-test breakpoint" candidate).

[clayborg]

I would remove all of the "need_adjust_break_address" and just always call that function in the architecture? Or only call it if m_skip_prologue is true? Seems like this logic should always happen. Seems like magic to only adjust the breakpoint address if the prologue byte size is zero or we aren't skipping the prologue. Feel free to pass any extra args down into AdjustBreakpointAddress() if needed (like m_skip_prologue or prologue_byte_size).

source/Breakpoint/BreakpointResolverName.cpp

385–390

Why not always do this and let the arch worry about the details of if it needs to be adjusted? Also, the AdjustBreakpointAddress(...) should probably just take a SymbolContext, not just a symbol. Why?
Because you might have stripped your symbol table and you might only have a function from debug info...

if (arch)
  arch->AdjustBreakpointAddress(sc, break_addr);

[lbianc]

I would remove all of the "need_adjust_break_address" and just always call that function in the architecture? Or only call it if m_skip_prologue is true? Seems like this logic should always happen. Seems like magic to only adjust the breakpoint address if the prologue byte size is zero or we aren't skipping the prologue. Feel free to pass any extra args down into AdjustBreakpointAddress() if needed (like m_skip_prologue or prologue_byte_size).

What if we remove the two identical blocks:

Adjust_Block:

if (m_skip_prologue && break_addr.IsValid()) {
  const uint32_t prologue_byte_size =
      sc.function->GetPrologueByteSize();
  if (prologue_byte_size)
    break_addr.SetOffset(break_addr.GetOffset() + prologue_byte_size);
  else
    need_adjust_break_address = true;
}else {
  need_adjust_break_address = true;
}

and create a new local function in BreakpointResolverName, like:

BreakpointResolverName::AdjustBreakpointAddress(SymbolContext &context, bool m_skip_prologue, Address break_addr){
}

so, inside the function, I would place the block I mentioned above and remove the need_adjust_break_address = true;.

Why not always do this and let the arch worry about the details of if it needs to be adjusted? Also, the AdjustBreakpointAddress(...) should probably just take a SymbolContext, not just a symbol. Why?
Because you might have stripped your symbol table and you might only have a function from debug info...

Do you mean put the Adjust_Block inside the arch->AdjustBreakpointAddress(? If so, what if arch is null? I don't know if this may happen.

[lbianc]

ping

[clayborg]

In D44437#1036993, @lbianc wrote:

I would remove all of the "need_adjust_break_address" and just always call that function in the architecture? Or only call it if m_skip_prologue is true? Seems like this logic should always happen. Seems like magic to only adjust the breakpoint address if the prologue byte size is zero or we aren't skipping the prologue. Feel free to pass any extra args down into AdjustBreakpointAddress() if needed (like m_skip_prologue or prologue_byte_size).

What if we remove the two identical blocks:

Adjust_Block:

if (m_skip_prologue && break_addr.IsValid()) {
  const uint32_t prologue_byte_size =
      sc.function->GetPrologueByteSize();
  if (prologue_byte_size)
    break_addr.SetOffset(break_addr.GetOffset() + prologue_byte_size);
  else
    need_adjust_break_address = true;
}else {
  need_adjust_break_address = true;
}

and create a new local function in BreakpointResolverName, like:

BreakpointResolverName::AdjustBreakpointAddress(SymbolContext &context, bool m_skip_prologue, Address break_addr){
}

so, inside the function, I would place the block I mentioned above and remove the need_adjust_break_address = true;.

Why not always do this and let the arch worry about the details of if it needs to be adjusted? Also, the AdjustBreakpointAddress(...) should probably just take a SymbolContext, not just a symbol. Why?
Because you might have stripped your symbol table and you might only have a function from debug info...

Do you mean put the Adjust_Block inside the arch->AdjustBreakpointAddress(? If so, what if arch is null? I don't know if this may happen.

If arch is NULL, then we just leave it as is. So none of the code would change and you would just add:

if (arch)
  arch->AdjustBreakpointAddress(sc, break_addr);

The arch specific code can adjust if needed based of either sc.function or sc.symbol (or both). No arch, then no arch specific adjustment. Much simpler.

[lbianc]

Modified as requested and added a new test case.

[lbianc]

I have added a new test case inside the TestMiBreak.py which is the file where we found the issue, and using the LLDB-mi I can disable the skip prologue, where the code can be tested.

[labath]

In D44437#1054762, @lbianc wrote:

I have added a new test case inside the TestMiBreak.py which is the file where we found the issue, and using the LLDB-mi I can disable the skip prologue, where the code can be tested.

Since the fix is in lldb code, I think this should be tested with an lldb test as well. This is particularly important as a lot of people/bots don't run lldb-mi tests nowadays due to their flakyness.

lldb-test breakpoint sounds like a perfect candidate for this test. Note that you can easily skip prologue from within lldb as well: breakpoint set --skip-prologue. I don't have an issue with keeping the lldb-mi test as well, if you think that adds any value..

source/Breakpoint/BreakpointResolverName.cpp

343–383

All of these else ifs make the logic here hard to follow. Could you factor this out into a function like ComputeBreakpointAddress or something?

[lbianc]

• Created function to calculate breakpoint address.
• Added test case in lldb-test

[lbianc]

• Added test case in lldb-test breakpoint.
• Code refactored and ComputeBreakpointAddress function created.

[labath]

lit/Breakpoint/ppc64-call-function.test

12 ↗

(On Diff #140949)

This looks very fragile. We don't want to depend on the number of instructions the compiler chooses to emit in the prologue. Could this be an assembly file again? (A minimal one, so remove all references to cstdio, printf and main from the cpp file before compiling it)

source/Breakpoint/BreakpointResolverName.cpp

343–383

That's not really what I had in mind. There's still many if's in this function, only now some of them are also duplicated in the helper function. What I wanted to do is to move the whole block into a function, to make it obvious that it's only purpose is to compute the break_addr and is_reexported values, and it's only input is the symbol context.

So, this function could be simplified to something like:
std::tie(break_addr, is_reexported) = ComputeBreakpointAddress(sc);

and the code in the function could take advantage of early returns to reduce the nesting level and make it obvious that the function does three different things, depending on the kind of information we have:

if (sc.block && sc.block->GetInlinedFunctionInfo()) {
  if (!sc.block->GetStartAddress(break_addr))
    break_addr.Clear();
  return {break_addr, false};
}

if (sc.function) {
  break_addr = sc.function->GetAddressRange().GetBaseAddress();
  if (!break_addr.IsValid())
    return {break_addr, false};
  const uint32_t prologue_byte_size = sc.function->GetPrologueByteSize();
  if (m_skip_prologue && prologue_byte_size)
      break_addr.SetOffset(break_addr.GetOffset() + prologue_byte_size);
  else if (arch)
      arch->AdjustBreakpointAddress(*sc.symbol, break_addr);
  return {break_addr, false};
}

...

(This also shows that you need two types of tests, one for function with debug info, and one without. The one without debug info could again be done via assembler (probably the same input as the existing test would suffice), to have it available everywhere.)

[lbianc]

lit/Breakpoint/ppc64-call-function.test

12 ↗

(On Diff #140949)

Ok, I agree this may be fragile, but I couldn't find a way to provide the information about the prologue size writing an assembly code, is there a way to do that?
If I cannot generate this information, and considering the test case that already exists (the one without debug info), which covers this changes as well, what kind of tests may be added?

[labath]

lit/Breakpoint/ppc64-call-function.test

12 ↗

(On Diff #140949)

Where does the debugger pick up the prologue size from? It should be some field in the .debug_line, .debug_info or something like that right?

In that case you can get the appropriate assembly by running "$CC -s -g yourfile.c".

(That's why I was insisting on a minimal source file, to avoid having large blobs of dwarf in the test when they're not actually needed. For this test, I am guessing a source file like int foo(int a) { return a; } will be sufficient.)

[lbianc]

lit/Breakpoint/ppc64-call-function.test

12 ↗

(On Diff #140949)

Ok, I got what you want, but I have found some issues:

• I cannot just have a single function, if so, the compiler does not generate the instructions of GEP, then I needed two functions, and one of them needs to call the other. Another possibility is creating one function, where the main calls it, but I would have to create the breakpoint in the main function, as only it will have the GEP and a prologue in this case.
• Using the llvm-mc does not work when the file has debug information, I have tested in ppc64le and x86_64 architectures and using both the lldb breaks when trying to add the breakpoint with segmentation fault. Did I miss something here, like any extra parameters need to be given?
• Using the clang to compile the .S file it needs a main, so I needed to add it in the .c file.

So, considering that, I'm going to create the input .S file with debug information of the following code:

int foo1(void) { return 1; }
int foo2(void) { return foo1() + 1; }

int main() { return 0; }

The test case without debug information is covered by ppc64-localentry.s, right? In this case, without debug information the prologue_end is not set, so the test that would be created is the same that is found in ppc64-localentry.test, i. e., breakpoint set -n lfunc.

[labath]

lit/Breakpoint/ppc64-call-function.test

12 ↗

(On Diff #140949)

Right, sorry, I forgot about the whole relocation problem. We will fail opening an unlinked .o file because it will have relocations we don't know how to handle (and that linker will typically remove for us).

It's probably time we start requiring lld so we have more reproducible tests (windows folks need that as well). I'll try to find some time for that, but in the mean time, i don't want to block your patch.

So let's go with your approach. You should be able to replace the "clang foo.S" invocation with a combination of llvm-mc + (system) ld, and then once we have lld we can just replace ld with lld and enable the test everywhere.

[lbianc]

• Added assembly test case with debug information.
• Modified refactor of breakpoint address calculation.

[labath]

lit/Breakpoint/ppc64-call-function.test

1 ↗

(On Diff #141703)

Thanks. This looks fine except for this requires line. As it stands now, the "powerpc" feature means "powerpc is a configured llvm target". Since you're using the system ld here, that is not enough. You'll need to modify lit/lit.cfg to add something like ppc64le-linux (similar to how we have armhf-linux) and use that here instead.

source/Breakpoint/BreakpointResolverName.cpp

436–439

This looks much better. Thanks!

[labath]

lit/Breakpoint/ppc64-call-function.test

1 ↗

(On Diff #141703)

To elaborate: Because we're using the system linker (for now), we need to make sure we are running on a ppc host. So we need a feature that reflects the host system instead of the configured targets.

[lbianc]

lit/Breakpoint/ppc64-call-function.test

1 ↗

(On Diff #141703)

Ok.
May I change the ppc64-localentry.test to requites ppc64le-linux as well or as it not using ld we can leave it as is?

[labath]

lit/Breakpoint/ppc64-call-function.test

1 ↗

(On Diff #141703)

The other test is fine. It does not use any capabilities of the host system, so the only thing we need for it is to have ppc64 targets configured in llvm.

[lbianc]

Cheking target architecture to run ppc64-call-functtion.test

[labath]

Thank you for bearing with me. This looks fine to me now, but I think Greg or Jim should take one more look at the final result.

[lbianc]

Thank you for bearing with me. This looks fine to me now, but I think Greg or Jim should take one more look at the final result.

No worries, that is how it should work. Thanks for reviewing!

[lbianc]

ping

[luporl]

Any update on this?
It seems it only need Greg or Jim to take a look at the final result, right?

[clayborg]

source/Breakpoint/BreakpointResolverName.cpp

438–439

I still question if AdjustBreakpointAddress should take a full SymbolContext instead of just a symbol. What if the symbol table was stripped and there is debug info for the function? We should be able to look at either the symbol or the function, but the function should always take precedence. Why? Some symbol tables don't have valid sizes on their symbols and that means symbols might synthetically set their lengths to the delta between symbols. It also seems like we might want to try to call AdjustBreakpointAddress ever if we skip the prologue?

[luporl]

source/Breakpoint/BreakpointResolverName.cpp

438–439

I think that passing SymbolContext instead of just a symbol would be better only if there is a chance that other architectures would benefit from it. For PPC64 Architecture plugin, only the symbol will be used by AdjustBreakpointAddress.

This is because PPC64's Local Entry Point (LEP) information is encoded in 3 bits of the "other" field of the symbol.
And LEP can only be obtained from this field, it is not present at debug info.

What if the symbol table was stripped and there is debug info for the function? We should be able to look at either the symbol or the function, but the function should always take precedence. Why? Some symbol tables don't have valid sizes on their symbols and that means symbols might synthetically set their lengths to the delta between symbols.

As mentioned above, for PPC64, LEP is not present in debug info. If there is no symbol info, we can do nothing, but just leave the breakpoint address unmodified.
The size is not taken into account by our AdjustBreakpointAddress implementation. As long as there is no invalid data at symbol's "other" field, it works. Remembering that not setting the corresponding "other" bits (i.e. leaving them as zero) means GEP = LEP, and then, also, there is nothing to adjust.

It also seems like we might want to try to call AdjustBreakpointAddress ever if we skip the prologue?

For PPC64, this is unnecessary. If the prologue is skipped, the breakpoint address will always be greater than or equal LEP.
Right now, calling it in this case would incorrectly add LEP offset bytes to the breakpoint address.
To make it into a no-op, AdjustBreakpointAddress would have to check if address == GEP and only adjust it in this case.
It seems better to just no call it in this case.

[luporl]

Hello @clayborg, any update on this?

Is it clear why, at least on PPC64, the SymbolContext is not needed, as the needed info is at the symbol, and only at the symbol?

[clayborg]

In D44437#1219196, @luporl wrote:

Hello @clayborg, any update on this?

Is it clear why, at least on PPC64, the SymbolContext is not needed, as the needed info is at the symbol, and only at the symbol?

It is fine for PPC64 to only require a symbol, but for other architectures, it might be able to use the function. Since we are designing APIs for all architectures here, we should send an entire symbol context just in case. It is fine for the PPC64 version to require just the symbol. but it should be grabbed from the symbol context

[luporl]

In D44437#1219616, @clayborg wrote:

In D44437#1219196, @luporl wrote:

Hello @clayborg, any update on this?

Is it clear why, at least on PPC64, the SymbolContext is not needed, as the needed info is at the symbol, and only at the symbol?

It is fine for PPC64 to only require a symbol, but for other architectures, it might be able to use the function. Since we are designing APIs for all architectures here, we should send an entire symbol context just in case. It is fine for the PPC64 version to require just the symbol. but it should be grabbed from the symbol context

I see, it makes sense. Ok, I'll do the change and post a new diff.

[luporl]

@lbianc is no longer working on this. I'll continue his work from where he left it.

[luporl]

• Changed parameter: Symbol -> SymbolContext

[luporl]

Thanks @clayborg! Can you please commit this change for me?

[luporl]

Is there anything pending on this yet?
It seems that we just need someone with write access to check in the changes, right?

[luporl]

ping

[luporl]

@clayborg, it seems this change ended up forgotten here. Can you check this in when you have the time? Thanks.

packages/Python/lldbsuite/test/tools/lldb-mi/breakpoint/TestMiBreak.py

def test_lldbmi_break_enable_disable(self):
         self.expect("\^done")
         self.expect(
             "=breakpoint-modified,bkpt={number=\"3\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"(?!0xffffffffffffffff)0x[0-9a-f]+\",func=\"ns::foo2\(\)\",file=\"main\.cpp\",fullname=\".+?main\.cpp\",line=\"\d+\",times=\"0\",original-location=\"ns::foo2\"}")
 
         self.runCmd("-exec-continue")
         self.expect("\^running")
         self.expect(
             "\*stopped,reason=\"breakpoint-hit\",disp=\"del\",bkptno=\"3\"")
+
+    @skipIfWindows  # llvm.org/pr24452: Get lldb-mi tests working on Windows
+    @skipIfFreeBSD  # llvm.org/pr22411: Failure presumably due to known thread races
+    @skipIfRemote   # We do not currently support remote debugging via the MI.
+    def test_lldbmi_break_in_prologue(self):
+        """Test that 'lldb-mi --interpreter' works when adding a breakpoint in the prologue."""
+
+        self.spawnLldbMi(args=None)
+
+        self.runCmd("-file-exec-and-symbols %s" % self.myexe)
+        self.expect("\^done")
+
+        self.runCmd("-break-insert main")
+        self.expect(
+            "\^done,bkpt={number=\"1\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"(?!0xffffffffffffffff)0x[0-9a-f]+\",func=\"main\"")
+        self.expect(
+            "=breakpoint-modified,bkpt={number=\"1\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"(?!0xffffffffffffffff)0x[0-9a-f]+\",func=\"main\",file=\"main\.cpp\",fullname=\".+?main\.cpp\",line=\"\d+\",times=\"0\",original-location=\"main\"}")
+
+        self.runCmd("-exec-run")
+        self.expect("\^running")
+        self.expect(
+            "=breakpoint-modified,bkpt={number=\"1\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"(?!0xffffffffffffffff)0x[0-9a-f]+\",func=\"main\",file=\"main\.cpp\",fullname=\".+?main\.cpp\",line=\"\d+\",times=\"0\",original-location=\"main\"}")
+        self.expect(
+            "\*stopped,reason=\"breakpoint-hit\",disp=\"del\",bkptno=\"1\"")
+
+        self.runCmd(
+            "-interpreter-exec console \"settings set target.skip-prologue on\"")
+        self.expect("\^done")
+
+        self.runCmd("-break-insert ns::foo1")
+        self.expect(
+            "\^done,bkpt={number=\"2\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"(?!0xffffffffffffffff)0x[0-9a-f]+\",func=\"ns::foo1\(\)\"")
+        self.expect(
+            "=breakpoint-modified,bkpt={number=\"2\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"(?!0xffffffffffffffff)0x[0-9a-f]+\",func=\"ns::foo1\(\)\",file=\"main\.cpp\",fullname=\".+?main\.cpp\",line=\"\d+\",times=\"0\",original-location=\"ns::foo1\"}")
+
+        self.runCmd("-break-insert ns::foo2")
+        self.expect(
+            "\^done,bkpt={number=\"3\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"(?!0xffffffffffffffff)0x[0-9a-f]+\",func=\"ns::foo2\(\)\"")
+        self.expect(
+            "=breakpoint-modified,bkpt={number=\"3\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"(?!0xffffffffffffffff)0x[0-9a-f]+\",func=\"ns::foo2\(\)\",file=\"main\.cpp\",fullname=\".+?main\.cpp\",line=\"\d+\",times=\"0\",original-location=\"ns::foo2\"}")
+
+        self.runCmd("-exec-continue")
+        self.expect("\^running")
+        self.expect(
+            "\*stopped,reason=\"breakpoint-hit\",disp=\"del\",bkptno=\"2\"")
+
+        self.runCmd("-exec-continue")
+        self.expect("\^running")
+        self.expect(
+            "\*stopped,reason=\"breakpoint-hit\",disp=\"del\",bkptno=\"3\"")

source/Breakpoint/BreakpointResolverName.cpp

for (size_t idx = 0; idx < num_functions; idx++) {
         idx--;
       }
     }
   }
 
   // Remove any duplicates between the function list and the symbol list
   SymbolContext sc;
   if (func_list.GetSize()) {
+    Architecture *arch = m_breakpoint->GetTarget().GetArchitecturePlugin();
     for (i = 0; i < func_list.GetSize(); i++) {
       if (func_list.GetContextAtIndex(i, sc)) {
         bool is_reexported = false;
 
         if (sc.block && sc.block->GetInlinedFunctionInfo()) {
           if (!sc.block->GetStartAddress(break_addr))
             break_addr.Clear();
         } else if (sc.function) {
           break_addr = sc.function->GetAddressRange().GetBaseAddress();
           if (m_skip_prologue && break_addr.IsValid()) {
             const uint32_t prologue_byte_size =
                 sc.function->GetPrologueByteSize();
-            if (prologue_byte_size)
+            if (prologue_byte_size) {
               break_addr.SetOffset(break_addr.GetOffset() + prologue_byte_size);
+            } else if (arch) {
+              arch->AdjustBreakpointAddress(*sc.symbol, break_addr);
+            }
+          } else if (arch) {
+            arch->AdjustBreakpointAddress(*sc.symbol, break_addr);
           }
+
         } else if (sc.symbol) {
           if (sc.symbol->GetType() == eSymbolTypeReExported) {
             const Symbol *actual_symbol =
                 sc.symbol->ResolveReExportedSymbol(m_breakpoint->GetTarget());
             if (actual_symbol) {
               is_reexported = true;
               break_addr = actual_symbol->GetAddress();
             }
           } else {
             break_addr = sc.symbol->GetAddress();
           }
 
           if (m_skip_prologue && break_addr.IsValid()) {
             const uint32_t prologue_byte_size =
                 sc.symbol->GetPrologueByteSize();
-            if (prologue_byte_size)
+            if (prologue_byte_size) {
               break_addr.SetOffset(break_addr.GetOffset() + prologue_byte_size);
-            else {
-              Architecture *arch =
-                  m_breakpoint->GetTarget().GetArchitecturePlugin();
-              if (arch)
-                arch->AdjustBreakpointAddress(*sc.symbol, break_addr);
+            } else if (arch) {
             }
+          } else if (arch) {
+            arch->AdjustBreakpointAddress(*sc.symbol, break_addr);
           }
         }
 
         if (break_addr.IsValid()) {
           if (filter.AddressPasses(break_addr)) {
             BreakpointLocationSP bp_loc_sp(
                 AddLocation(break_addr, &new_location));
             bp_loc_sp->SetIsReExported(is_reexported);
             if (bp_loc_sp && new_location && !m_breakpoint->IsInternal()) {
               if (log) {
                 StreamString s;
                 bp_loc_sp->GetDescription(&s, lldb::eDescriptionLevelVerbose);
                 log->Printf("Added location: %s\n", s.GetData());
               }
             }
           }
         }
void BreakpointResolverName::GetDescription(Stream *s) {
   }
 }
 
 void BreakpointResolverName::Dump(Stream *s) const {}
 
 lldb::BreakpointResolverSP
 BreakpointResolverName::CopyForBreakpoint(Breakpoint &breakpoint) {
   lldb::BreakpointResolverSP ret_sp(new BreakpointResolverName(*this));
   ret_sp->SetBreakpoint(&breakpoint);
   return ret_sp;
 }