Re: [Lldb-commits] [PATCH] D27017: Support more report types in AddressSanitizerRuntime.cpp
.Case("stack-use-after-return", "Use of returned stack memory")
Maybe "Use of stack memory after (function) return"? (i couldn't decide
whether to include "function". Either delete it or delete the parens.
This is a very minor nit, so I'm ok with keeping the current wording if you
prefer that.
On Fri, 2 Dec 2016 at 20:04, Kuba (Brecka) Mracek via Phabricator <
revi...@reviews.llvm.org> wrote:
> kubabrecka added a comment.
>
>
>
> In https://reviews.llvm.org/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?
>
>
>
>
>
> 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] [PATCH] D27017: Support more report types in AddressSanitizerRuntime.cpp
filcab added inline comments.
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") {
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. :-)
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] [PATCH] D27017: Support more report types in AddressSanitizerRuntime.cpp
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
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D22463: [RFC] Moving to GitHub Proposal: NOT DECISION!
filcab added a comment. Thanks a lot for working on this! Filipe https://reviews.llvm.org/D22463 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D22463: [RFC] Moving to GitHub Proposal: NOT DECISION!
filcab added a subscriber: filcab.
filcab added a comment.
What about branches? I'm guessing we should expect the usual release branches.
But will any person be able to create a branch? Will there be a policy, if this
is the case? Is the policy enforceable?
Comment at: docs/Proposals/GitHub.rst:122
@@ +121,3 @@
+of understanding the *sequence* in which commits were added by using the
+``git rev-list --count hash`` or ``git describe hash`` commands.
+
How easy will it be to clone the "aggregated" repo, and then get some (but not
all) of the submodules?
Comment at: docs/Proposals/GitHub.rst:130
@@ +129,3 @@
+* Individual projects' history will be broken (linear, but local), and we need
+ the umbrella project (using submodules) to have the same view as we had in
SVN.
+
I wouldn't call it broken.
Won't it have the same end result as having a checkout per project and simply
updating them close to each other?
Basically, it won't be "any more broken" than using this method for updating:
```
#!/bin/bash
for dir in llvm/{,tools/{clang,lld},projects/{libcxx,libcxxabi,compiler-rt}}; do
# (cd $dir && svn up) # for SVN
(cd $dir && git checkout master && git pull) # for git
done
```
https://reviews.llvm.org/D22463
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
