Support more report types in AddressSanitizerRuntime.cpp

[kubamracek]

Summary

https://reviews.llvm.org/D27012 will add more report types into ASan that will be reported via the debugging API. This patch in LLDB provides human-friendly bug descriptions.

[clayborg]

Looks fine. One question though: do we always have a shared library that contains the ASAN runtime? If so, then we can actually put the array of short to long description into the ASAN dylib and then we read this array as a global variable. This would allow you to add as many descriptions as you want directly in your ASAN code and have LLDB detect all of the new strings. Or, if there is a function you can call with the short description that returns the long description, then we can still have the ASAN code contain all of the strings.

[filcab]

I have some minor fixes I'd like to see.
If it's prefixed by "Nit: " it's a really minor one and I'm ok with it as is if that's what you prefer.

Thank you,
Filipe

source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp

220 ↗

(On Diff #78979)

Not necessarily recursion. There's also stack variables. I'd omit the stuff in parenthesis.

224 ↗

(On Diff #78979)

Nit: "Wild jump" is probably better than "wild pointer jump", no?

226 ↗

(On Diff #78979)

Nit: I'd probably use "Write through wild pointer ..." (same for the others)

234 ↗

(On Diff #78979)

Nit: Phrasing seems off. I think "Double free detected" is easier to read and more explicit.

236 ↗

(On Diff #78979)

Nit: Maybe "Deallocation size mismatch detected"? The user isn't deallocating a "size".

238 ↗

(On Diff #78979)

s/address/memory region/?

242 ↗

(On Diff #78979)

Maybe "Call to malloc_usable_size with not owned pointer detected"?
Unsure if it's obvious why it's "invalid". Same for the one below.

246 ↗

(On Diff #78979)

I think this really needs additional detail. It doesn't seem very meaningful as a sentence.
Maybe "Overlapping memory ranges in function that doesn't allow them (detected?)" or something?
My suggestion can be improved too, for sure :-)

250 ↗

(On Diff #78979)

Maybe "Invalid parameters to call to __sanitizer_annotate_contiguous_container" is more explicit?
Maybe also a good solution for the similar cases above.

252 ↗

(On Diff #78979)

"One Definition Rule violation"
It's not an initialization order problem.

[kubamracek]

Thanks for these comments, I'll have them run by more people before committing this (I'm not a native English speaker). These really need to be as user-focused as possible, so I'm really against stuff like "not owned pointer", because that's not something we should expect the user to understand. Also note that we're not using these as a replacement of full reports, we're showing them as a quick description in addition to ASan reports.

source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp

220 ↗

(On Diff #78979)

Multiple times have I seen that people read "stack overflow" as "stack buffer overflow" and they spend a lot of time trying to find what buffer was actually overflown... I'd like to somehow point that out. Ideas?

[filcab]

source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp

220 ↗

(On Diff #78979)

Maybe instead of "recursion too deep" have "stack space exhausted" or something like that?
I've seen stack overflow errors on as few as 10 (maybe even fewer) stack frames (with big objects). ASan is also more likely to make this a problem. I think seeing "recursion too deep" but having only a dozen frames is probably confusing.
Not that "stack space exhausted" is much better, but I think it's less likely to be misleading.

And yes, please ask native speakers too, as I'm not one either. :-)

[zaks.anna]

Here is another pass at this:

source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp

198 ↗

(On Diff #78979)

Kuba would like to drop all of these "detected" that do not add anything and are just used as filler words in all of the description messages. This will make some of them into non-complete sentences, but keeping them short is more user friendly and gets the point across just fine.

220 ↗

(On Diff #78979)

We could just say "Stack space exhausted" with no parentheses.

222 ↗

(On Diff #78979)

Drop "detected" or "Dereference of null pointer".

224 ↗

(On Diff #78979)

How about "Jump to a wild address". "Wild jump" sounds like a term, but I am not familiar with it.

226 ↗

(On Diff #78979)

"Write through", "Access through" but "Read from" (as per the English speaker on our team :)).

234 ↗

(On Diff #78979)

Double free is a bit of a jargon, I think it's better to be explicit:
"Deallocation of freed memory"

236 ↗

(On Diff #78979)

Is this always about "new" and "delete"? If so, we could try to be more explicit about it "The size of deleted object does not match the size at allocation"

"Deallocation size different than allocation size"?

238 ↗

(On Diff #78979)

"Deallocation of non-allocated memory"

242 ↗

(On Diff #78979)

"Invalid argument to malloc_usable_size"? And "Invalid argument to __sanitizer_get_allocated_size" below.

246 ↗

(On Diff #78979)

How about this: "Call to function disallowing overlapping memory ranges"

248 ↗

(On Diff #78979)

Negative size used when accessing memory

250 ↗

(On Diff #78979)

"Invalid arguments to __sanitizer_annotate_contiguous_container"

252 ↗

(On Diff #78979)

Is it possible to use something similar to what clang uses, for example, "external variable %0 defined in multiple translation units" (from ./include//clang/Basic/DiagnosticASTKinds.td)?

Ex: "Symbol defined in multiple translation units"

254 ↗

(On Diff #78979)

Comparison or arithmetic on pointers from different memory regions

[filcab]

I'd mostly use Anna's suggestions. Kuba: Can you remove the "detected" from the messages and refresh the patch, just so we can have one last look? Otherwise, if you think it's good enough, commit with these changes and we can do post-commit review.

source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp

198 ↗

(On Diff #78979)

Agree.

220 ↗

(On Diff #78979)

Yes.

222 ↗

(On Diff #78979)

I'd go for the second option.

224 ↗

(On Diff #78979)

"Jump to non-executable address" (basically, it's either non-readable (or unmapped) or non-executable. But I don't think we need to have those distinctions in this error message)? If not, I'm fine with Anna's suggestion too.

226 ↗

(On Diff #78979)

Heh. Right.

234 ↗

(On Diff #78979)

Good.

236 ↗

(On Diff #78979)

"... different from..."?
Otherwise, I like it.

[kubamracek]

Updating the patch.

[kubamracek]

In D27017#604265, @clayborg wrote:

Looks fine. One question though: do we always have a shared library that contains the ASAN runtime? If so, then we can actually put the array of short to long description into the ASAN dylib and then we read this array as a global variable. This would allow you to add as many descriptions as you want directly in your ASAN code and have LLDB detect all of the new strings. Or, if there is a function you can call with the short description that returns the long description, then we can still have the ASAN code contain all of the strings.

We do have the dylib with the runtime any time this code is run. However, I don't think these should live in the ASan runtime, because they should be considered as more high-level user-visible strings. The ASan runtime itself produces detailed technical reports that don't use the wording used here. For example, the console report will just say "Invalid pointer pair" assuming the user knows what that means. When using LLDB (and/or Xcode) we should provide less technical descriptions.

Anyway, I think this asks for a longer discussion. Are you okay with leaving these strings here for now?

[zaks.anna]

Looks great to me. Thank you Kuba!!!

[filcab]

LGTM

(I commented on a minor nit. It might just be me, so feel free to keep the current wording if you feel it's preferred)

[kubamracek]

In D27017#611894, @filcab wrote:

LGTM

(I commented on a minor nit. It might just be me, so feel free to keep the current wording if you feel it's preferred)

I'm not seeing this comment. Can you post it again?

[kubamracek]

Thanks for reviewing this!

lldb/trunk/source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp

 #include "lldb/Expression/UserExpression.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/Target/InstrumentationRuntimeStopInfo.h"
 #include "lldb/Target/StopInfo.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/Thread.h"
 
+#include "llvm/ADT/StringSwitch.h"
+
 using namespace lldb;
 using namespace lldb_private;
 
 lldb::InstrumentationRuntimeSP
 AddressSanitizerRuntime::CreateInstance(const lldb::ProcessSP &process_sp) {
   return InstrumentationRuntimeSP(new AddressSanitizerRuntime(process_sp));
 }
 

 }
 
 std::string
 AddressSanitizerRuntime::FormatDescription(StructuredData::ObjectSP report) {
   std::string description = report->GetAsDictionary()
                                 ->GetValueForKey("description")
                                 ->GetAsString()
                                 ->GetValue();
-  if (description == "heap-use-after-free") {
-    return "Use of deallocated memory detected";
-  } else if (description == "heap-buffer-overflow") {
-    return "Heap buffer overflow detected";
-  } else if (description == "stack-buffer-underflow") {
-    return "Stack buffer underflow detected";
-  } else if (description == "initialization-order-fiasco") {
-    return "Initialization order problem detected";
-  } else if (description == "stack-buffer-overflow") {
-    return "Stack buffer overflow detected";
-  } else if (description == "stack-use-after-return") {
-    return "Use of returned stack memory detected";
-  } else if (description == "use-after-poison") {
-    return "Use of poisoned memory detected";
-  } else if (description == "container-overflow") {
-    return "Container overflow detected";
-  } else if (description == "stack-use-after-scope") {
-    return "Use of out-of-scope stack memory detected";
-  } else if (description == "global-buffer-overflow") {
-    return "Global buffer overflow detected";
-  } else if (description == "unknown-crash") {
-    return "Invalid memory access detected";
-  }
-
+  return llvm::StringSwitch<std::string>(description)
+      .Case("heap-use-after-free", "Use of deallocated memory")
+      .Case("heap-buffer-overflow", "Heap buffer overflow")
+      .Case("stack-buffer-underflow", "Stack buffer underflow")
+      .Case("initialization-order-fiasco", "Initialization order problem")
+      .Case("stack-buffer-overflow", "Stack buffer overflow")
+      .Case("stack-use-after-return", "Use of stack memory after return")
+      .Case("use-after-poison", "Use of poisoned memory")
+      .Case("container-overflow", "Container overflow")
+      .Case("stack-use-after-scope", "Use of out-of-scope stack memory")
+      .Case("global-buffer-overflow", "Global buffer overflow")
+      .Case("unknown-crash", "Invalid memory access")
+      .Case("stack-overflow", "Stack space exhausted")
+      .Case("null-deref", "Dereference of null pointer")
+      .Case("wild-jump", "Jump to non-executable address")
+      .Case("wild-addr-write", "Write through wild pointer")
+      .Case("wild-addr-read", "Read from wild pointer")
+      .Case("wild-addr", "Access through wild pointer")
+      .Case("signal", "Deadly signal")
+      .Case("double-free", "Deallocation of freed memory")
+      .Case("new-delete-type-mismatch",
+            "Deallocation size different from allocation size")
+      .Case("bad-free", "Deallocation of non-allocated memory")
+      .Case("alloc-dealloc-mismatch",
+            "Mismatch between allocation and deallocation APIs")
+      .Case("bad-malloc_usable_size", "Invalid argument to malloc_usable_size")
+      .Case("bad-__sanitizer_get_allocated_size",
+            "Invalid argument to __sanitizer_get_allocated_size")
+      .Case("param-overlap",
+            "Call to function disallowing overlapping memory ranges")
+      .Case("negative-size-param", "Negative size used when accessing memory")
+      .Case("bad-__sanitizer_annotate_contiguous_container",
+            "Invalid argument to __sanitizer_annotate_contiguous_container")
+      .Case("odr-violation", "Symbol defined in multiple translation units")
+      .Case(
+          "invalid-pointer-pair",
+          "Comparison or arithmetic on pointers from different memory regions")
       // for unknown report codes just show the code
-  return description;
+      .Default("AddressSanitizer detected: " + description);
 }
 
 bool AddressSanitizerRuntime::NotifyBreakpointHit(
     void *baton, StoppointCallbackContext *context, user_id_t break_id,
     user_id_t break_loc_id) {
   assert(baton && "null baton");
   if (!baton)
     return false;