[Lldb-commits] [PATCH] D102428: [StopInfoMachException] Summarize arm64e BLRAx/LDRAx auth failures

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

I think some of my questions are too detailed for this initial patch, I'm sure we can improve the diagnostics over time as situations come up. Glad to see this being upstreamed.

Not very knowledgeable on Mach specifics so I'll leave the final review to others.



================
Comment at: lldb/packages/Python/lldbsuite/test/functionalities/ptrauth_diagnostics/BLRAA_error/blraa.c:19
+
+// Before:
+#if 0
----------------
vsk wrote:
> DavidSpickett wrote:
> > What is the purpose of the `// Before:` blocks here? Simply to give you a clue what happened if the test fails, or something else?
> > 
> > (I guess I'm really saying "Before", before what exactly)
> These 'Before' blocks show the reader how lldb interpreted auth-related exceptions prior to this change, and show the expected codegen (handy if the test ever regresses).
Cool, but could you reword it? something like:
```
// Without PAuth diagnostics we got:
```

Or "expected codegen and lldb output used to be", so readers have context if they're just looking at this test because it failed and they're not working on pauth specifically.

Also you could put the text in comments instead of an ifdef: (`///` instead of `//` so they look different to check lines)
```
/// * thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x2000000100007f9c)
///    frame #0: 0x0000000100007f9c blraa2`foo
```

I don't think FileCheck will trip up on anything in this.


================
Comment at: lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp:126
+    Address brk_address;
+    if (!target.ResolveLoadAddress(fixed_bad_address, brk_address))
+      return false;
----------------
vsk wrote:
> DavidSpickett wrote:
> > What does it mean here that the address failed to resolve?
> It's possible that lldb doesn't know about the image the fixed address points to (it could be a garbage value). In this case we conservatively don't hint that there's a ptrauth issue.
So in that case we would report stopped due to a breakpoint, that's a special pac breakpoint but no pointer authentication issue? Isn't that confusing for the user?

Maybe not because it's hinting at accidental corruption vs. deliberate misdirection, you probably have the experiences to inform that.

This is an improvement as is so no need to change it I'm just curious.

Can you add a test for this situation? Assuming you can find an address you know would never be valid.


================
Comment at: lldb/test/API/functionalities/ptrauth_diagnostics/BRAA_error/braa.c:19
+}
+
+#if 0
----------------
This one needs a comment like the others:
```
/// Without PAuth diagnostics ...
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102428/new/

https://reviews.llvm.org/D102428