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

[Filipe Cabecinhas via lldb-commits]

filcab added a comment.

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



================
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") {
----------------
Not necessarily recursion. There's also stack variables. I'd omit the stuff in parenthesis.


================
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") {
----------------
Nit: "Wild jump" is probably better than "wild pointer jump", no?


================
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") {
----------------
Nit: I'd probably use "Write through wild pointer ..." (same for the others)


================
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") {
----------------
Nit: Phrasing seems off. I think "Double free detected" is easier to read and more explicit.


================
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") {
----------------
Nit: Maybe "Deallocation size mismatch detected"? The user isn't deallocating a "size".


================
Comment at: source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp:238
+  } else if (description == "bad-free") {
+    return "Attempt to free a non-allocated address detected";
+  } else if (description == "alloc-dealloc-mismatch") {
----------------
s/address/memory region/?



================
Comment at: source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp:242
+  } else if (description == "bad-malloc_usable_size") {
+    return "Invalid call to malloc_usable_size detected";
+  } else if (description == "bad-__sanitizer_get_allocated_size") {
----------------
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.


================
Comment at: source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp:246
+  } else if (description == "param-overlap") {
+    return "Overlapping memory ranges detected";
+  } else if (description == "negative-size-param") {
----------------
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 :-)


================
Comment at: source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp:250
+  } else if (description == "bad-__sanitizer_annotate_contiguous_container") {
+    return "Invalid call to __sanitizer_annotate_contiguous_container detected";
+  } else if (description == "odr-violation") {
----------------
Maybe "Invalid parameters to call to __sanitizer_annotate_contiguous_container" is more explicit?
Maybe also a good solution for the similar cases above.


================
Comment at: source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp:252
+  } else if (description == "odr-violation") {
+    return "Initialization order problem detected";
+  } else if (description == "invalid-pointer-pair") {
----------------
"One Definition Rule violation"
It's not an initialization order problem.


Repository:
  rL LLVM

https://reviews.llvm.org/D27017