[Lldb-commits] [PATCH] D27017: Support more report types in AddressSanitizerRuntime.cpp

[Filipe Cabecinhas via Phabricator via lldb-commits]

filcab added a comment.

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.



================
Comment at: source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp:198
   if (description == "heap-use-after-free") {
     return "Use of deallocated memory detected";
   } else if (description == "heap-buffer-overflow") {
----------------
zaks.anna wrote:
> 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.
Agree.


================
Comment at: source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp:220
+  } else if (description == "stack-overflow") {
+    return "Stack overflow detected (recursion too deep)";
+  } else if (description == "null-deref") {
----------------
zaks.anna wrote:
> filcab wrote:
> > kubabrecka wrote:
> > > filcab wrote:
> > > > Not necessarily recursion. There's also stack variables. I'd omit the stuff in parenthesis.
> > > 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?
> > 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. :-)
> > 
> We could just say "Stack space exhausted" with no parentheses.
Yes.


================
Comment at: source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp:222
+  } else if (description == "null-deref") {
+    return "Null pointer dereference detected";
+  } else if (description == "wild-jump") {
----------------
zaks.anna wrote:
> Drop "detected" or "Dereference of null pointer".
> 
I'd go for the second option.


================
Comment at: source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp:224
+  } else if (description == "wild-jump") {
+    return "Wild pointer jump detected";
+  } else if (description == "wild-addr-write") {
----------------
zaks.anna wrote:
> filcab wrote:
> > Nit: "Wild jump" is probably better than "wild pointer jump", no?
> How about "Jump to a wild address". "Wild jump" sounds like a term, but I am not familiar with  it.
"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.


================
Comment at: source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp:226
+  } else if (description == "wild-addr-write") {
+    return "Write to a wild pointer detected";
+  } else if (description == "wild-addr-read") {
----------------
zaks.anna wrote:
> filcab wrote:
> > Nit: I'd probably use "Write through wild pointer ..." (same for the others)
> "Write through", "Access through" but "Read from" (as per the English speaker on our team :)).
Heh. Right.


================
Comment at: source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp:234
+  } else if (description == "double-free") {
+    return "Attempt to deallocate a freed object detected";
+  } else if (description == "new-delete-type-mismatch") {
----------------
zaks.anna wrote:
> filcab wrote:
> > Nit: Phrasing seems off. I think "Double free detected" is easier to read and more explicit.
> Double free is a bit of a jargon, I think it's better to be explicit:
> "Deallocation of freed memory"
Good.


================
Comment at: source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp:236
+  } else if (description == "new-delete-type-mismatch") {
+    return "Deallocation of a wrong size detected";
+  } else if (description == "bad-free") {
----------------
zaks.anna wrote:
> filcab wrote:
> > Nit: Maybe "Deallocation size mismatch detected"? The user isn't deallocating a "size".
> 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"?
"... different from..."?
Otherwise, I like it.


Repository:
  rL LLVM

https://reviews.llvm.org/D27017