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@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@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits >
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits