[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

[Brooks Moses via Phabricator via cfe-commits]

brooksmoses added a comment.

I have noticed two things when attempting to release LLVM with this revision 
internally at Google:

1. It's catching real bugs, all in constructors where someone wrote "member_ = 
member_" when they meant "member_ = member".

2. It's catching at least as many cases of tests where people are intentionally 
testing that self-assignment doesn't corrupt the data values.

Thus, this seems valuable but problematic, and the problems mean that initially 
we're facing turning off -Wself-assign completely until this is resolved.  
That's definitely an issue, and at least means that this needs to be placed 
under its own -W option.  And the real bugs it's finding seem to be very 
specific, and could be found by a more-focused warning.

Further, I would note that most warnings of this sort have some canonical way 
of arranging the code to avoid the warning -- for instance, casting an unused 
variable to "void" to create a no-op expression and thereby avoid the "unused 
variable" warning.  This warning doesn't seem to have one; "var = (var)", for 
instance, should IMO turn it off but doesn't.

(To add to that, we have a source file that does a lot of "var = var" to 
silence unused-variable warnings, so this breaks that on top of not having its 
own source "workaround".  Ick.)


Repository:
  rC Clang

https://reviews.llvm.org/D44883



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

[Brooks Moses via Phabricator via cfe-commits]

brooksmoses added a comment.

Further note: I'm noticing that nearly all the signal is from 
-Wself-assign-field and all the noise is from -Wself-assign.  So that would be 
one obvious way to make this higher-signal in what's enabled in -Wall, if that 
were a desire.


Repository:
  rC Clang

https://reviews.llvm.org/D44883



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

[Brooks Moses via Phabricator via cfe-commits]

brooksmoses added a comment.

Some further statistics, now that I've done a full cleanup on our code:

8 actual bugs newly found by -Wself-assign-field.  (Thank you!)
2 actual bugs newly found by -Wself-assign
6 instances of redundant code newly found by -Wself-assign
90 (approximately) instances of intentional self assignments in tests that now 
need "*&" annotations.

That seems like an awfully large amount of noise for the -Wself-assign part of 
this, and I continue feeling rather dubious about whether it should be part of 
-Wall.  By contrast, the -Wself-assign-field part has been entirely true 
positives.


Repository:
  rC Clang

https://reviews.llvm.org/D44883



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

[Brooks Moses via Phabricator via cfe-commits]

brooksmoses added a comment.

A further concern about this in the general case from the reviewer of one of my 
test-cleanup changes: The "var = *&var" idiom is not necessarily equivalent to 
"var = var" in cases of user-defined types, because operator& may be overloaded.


Repository:
  rC Clang

https://reviews.llvm.org/D44883



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

[Brooks Moses via Phabricator via cfe-commits]

brooksmoses added a comment.

Thanks for the summary, John.  To confirm, I found two examples of bugs 
involving local variables, as well as the field-based examples.  And, indeed, 
all of the false positives were in unit tests.


Repository:
  rC Clang

https://reviews.llvm.org/D45685



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D17993: [CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments.

[Brooks Moses via Phabricator via cfe-commits]

brooksmoses added a comment.

So, I have bad news: This causes OpenJDK to segfault.  The relevant code is 
here:
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/memory/arena.cpp#L311

  void Arena::destruct_contents() {
if (UseMallocOnly && _first != NULL) {
  char* end = _first->next() ? _first->top() : _hwm;
  free_malloced_objects(_first, _first->bottom(), end, _hwm);
}
// reset size before chop to avoid a rare racing condition
// that can have total arena memory exceed total chunk memory
set_size_in_bytes(0);
_first->chop();
reset();
  }

I've also seen a segfault in Verilator that root-causes to this patch, though I 
haven't yet tracked that down to the source code.

I hate to say it, but is this a significant enough problem to call for a 
(temporary, I hope) rollback?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D17993/new/

https://reviews.llvm.org/D17993

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D17993: [CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments.

[Brooks Moses via Phabricator via cfe-commits]

brooksmoses added a comment.

Aha, okay.  I hadn't realized that this optimization had a 
`-fno-delete-null-pointer-checks` option to disable it.  I agree that since 
that's available there's no call for a rollback.

(I'll also make sure upstream bugs are filed for the cases in OpenJDK and 
Verilator that I know about, though I imagine those are likely to take a little 
while to make it out into the ecosystem.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D17993/new/

https://reviews.llvm.org/D17993

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131351: [C] Default implicit function pointer conversions diagnostic to be an error

[Brooks Moses via Phabricator via cfe-commits]

brooksmoses added a comment.

For the record, so far we've seen this showing up in the following:

- A case in a notoriously warning-heavy third-party library where we'd 
backported a file from a newer version and didn't quite fix all the internal 
API mismatches.
- A ten-year-old bug in a local patch to another third-party library, where a 
function-pointer parameter was defined as returning a `void` and then assigned 
to a function-pointer that returns a `void *`.
- A probably-innocuous bug in a local patch to yet another third-party library, 
where we were passing an `int foo(char *, char *)` function to GLIBC's `qsort`, 
which expects a function with a signature of `int foo(void *, void *)`.
- A case where 
https://gitlab.freedesktop.org/pixman/pixman/-/commit/e0d4403e78a7af8f4be4110ae25e9c3d1ac93a78
 wasn't applied to our local version.  This is also probably an innocuous case, 
not a "real" bug.
- A case where SciPy's extension has a function that uses `void *` for a `FILE 
*` pointer 
(https://github.com/scipy/scipy/blob/main/scipy/spatial/_qhull.pyx#L187, second 
argument) while the corresponding C code's function has a real `FILE *` pointer 
(https://github.com/qhull/qhull/blob/master/src/libqhull_r/io_r.h#L97).  The 
SciPy function also uses a `void *` for an argument of a struct type, which 
seems rather odd to me given that it just defined the type two lines earlier.

So, real bugs in 40% of cases, I'd say.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131351/new/

https://reviews.llvm.org/D131351

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156821: [CodeGen] [ubsan] Respect integer overflow handling in abs builtin

[Brooks Moses via Phabricator via cfe-commits]

brooksmoses added a comment.

Just as an FYI note, this found a bug in one of our random-number generators 
that was taking a random 32-bit integer and applying abs() to it.  Thank you!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156821/new/

https://reviews.llvm.org/D156821

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75844: [clang] Set begin loc on GNU attribute parsed attrs

[Brooks Moses via Phabricator via cfe-commits]

brooksmoses added a comment.

FWIW, this now causes Clang to produce an error on this code, when it didn't 
before:

  using namespace::foo __attribute__((deprecated("message")));

I discussed this with Richard Smith, who points out that GCC does not accept 
this and it's not permitted according to the C++ standard, so this should 
probably be considered an accidental fix of a longstanding bug.   With that 
said, would it be useful to add this as a tested case?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75844/new/

https://reviews.llvm.org/D75844

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99790: [CGCall] Annotate `this` argument with alignment

[Brooks Moses via Phabricator via cfe-commits]

brooksmoses added a comment.

As a heads up, I'm seeing segfaults on internal code as a result of this 
change, as well as errors in Eigen's unalignedassert.cpp test (specifically, 
this line asserts: 
https://github.com/madlib/eigen/blob/master/test/unalignedassert.cpp#L151).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99790/new/

https://reviews.llvm.org/D99790

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99790: [CGCall] Annotate `this` argument with alignment

[Brooks Moses via Phabricator via cfe-commits]

brooksmoses added a comment.

In D99790#2677919 , @lebedev.ri wrote:

> In D99790#2677917 , @brooksmoses 
> wrote:
>
>> As a heads up, I'm seeing segfaults on internal code as a result of this 
>> change, as well as errors in Eigen's unalignedassert.cpp test (specifically, 
>> this line asserts: 
>> https://github.com/madlib/eigen/blob/master/test/unalignedassert.cpp#L151).
>
> Would be good to have a small standalone reproducer.
> Not really sure how we can end up with a misaligned `this`, but it sounds 
> like UB.

Indeed, it's looking like all of the various segfaults are resulting from 
undefined behavior, just like the Eigen assert was (per @jyknight's comment).  
One of the segfaults is in the OpenJDK runtime -- albeit our internal copy, so 
it's possible it might not be in the external versions -- so that's fun.  
Luckily it shows up in the bootstrapping part of the build, rather than lying 
in wait to bite people after it's deployed.

In any case, thanks for the quick reply, and I'll figure out a small reproducer 
if we find something that isn't UB.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99790/new/

https://reviews.llvm.org/D99790

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99790: [CGCall] Annotate `this` argument with alignment

[Brooks Moses via Phabricator via cfe-commits]

brooksmoses added a comment.

In D99790#2678674 , @lebedev.ri wrote:

> In D99790#2678384 , @brooksmoses 
> wrote:
>
>> In any case, thanks for the quick reply, and I'll figure out a small 
>> reproducer if we find something that isn't UB.
>
> Nono, you misunderstand, i want the samples *with* UB.
> I will then revert this, and add UBSan check to catch that UB first.

Oh!  Okay, will do.  Do you still want the samples even if UBSan catches them?

Meanwhile, would you be open to providing a gating flag to turn this off, so we 
can roll it out generally while turning it off for the specific things that 
have issues until we can fix them?  So far, what we're finding is caught by 
UBSan but not very trivial to fix.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99790/new/

https://reviews.llvm.org/D99790

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits