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

[Zachary Turner via lldb-commits]

Not going to block the CL over this, but consider using
llvm::StringSwitch<> for improving the readability of this rather large if
/ else block.

On Mon, Nov 28, 2016 at 6:14 PM Anna Zaks via Phabricator via lldb-commits <
lldb-commits at lists.llvm.org> wrote:

> zaks.anna added a comment.
>
> Here is another pass at this:
>
>
>
> ================
> 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") {
> ----------------
> 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.
>
>
> ================
> 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") {
> ----------------
> 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.
>
>
> ================
> Comment at:
> source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp:222
> +  } else if (description == "null-deref") {
> +    return "Null pointer dereference detected";
> +  } else if (description == "wild-jump") {
> ----------------
> Drop "detected" or "Dereference of null pointer".
>
>
>
> ================
> 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") {
> ----------------
> 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.
>
>
> ================
> 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") {
> ----------------
> 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 :)).
>
>
> ================
> 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") {
> ----------------
> 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"
>
>
> ================
> 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") {
> ----------------
> 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"?
>
>
> ================
> 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") {
> ----------------
> filcab wrote:
> > s/address/memory region/?
> >
> "Deallocation of non-allocated memory"
>
>
> ================
> 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") {
> ----------------
> filcab wrote:
> > 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.
> "Invalid argument to malloc_usable_size"?  And "Invalid argument to
> __sanitizer_get_allocated_size" 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") {
> ----------------
> filcab wrote:
> > 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 :-)
> How about this: "Call to function disallowing overlapping memory ranges"
>
>
>
>
> ================
> Comment at:
> source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp:248
> +  } else if (description == "negative-size-param") {
> +    return "Negative size memory access detected";
> +  } else if (description ==
> "bad-__sanitizer_annotate_contiguous_container") {
> ----------------
> Negative size used when accessing memory
>
>
> ================
> 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") {
> ----------------
> filcab wrote:
> > Maybe "Invalid parameters to call to
> __sanitizer_annotate_contiguous_container" is more explicit?
> > Maybe also a good solution for the similar cases above.
> "Invalid arguments to __sanitizer_annotate_contiguous_container"
>
>
> ================
> 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") {
> ----------------
> filcab wrote:
> > "One Definition Rule violation"
> > It's not an initialization order problem.
> 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"
>
>
> ================
> Comment at:
> source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp:254
> +  } else if (description == "invalid-pointer-pair") {
> +    return "Invalid pointer pair in arithmetic operation detected";
>    }
> ----------------
> Comparison or arithmetic on pointers from different memory regions
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D27017
>
>
>
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20161129/55c3afa0/attachment-0001.html>