This is an archive of the discontinued LLVM Phabricator instance.

[APSInt][OpenMP] Fix isNegative, etc. for unsigned types
ClosedPublic

Authored by jdenny on Mar 22 2019, 12:47 PM.

Details

Summary

Without this patch, APSInt inherits APInt::isNegative, which merely
checks the sign bit without regard to whether the type is actually
signed. isNonNegative and isStrictlyPositive call isNegative and so
are also affected.

This patch adjusts APSInt to override isNegative, isNonNegative, and
isStrictlyPositive with implementations that consider whether the type
is signed.

A large set of Clang OpenMP tests are affected. Without this patch,
these tests assume that true is not a valid argument for clauses
like collapse. Indeed, true fails APInt::isStrictlyPositive but
not APSInt::isStrictlyPositive. This patch adjusts those tests to
assume true should be accepted.

This patch also adds tests revealing various other similar fixes due
to APSInt::isNegative calls in Clang's ExprConstant.cpp and
SemaExpr.cpp: ++ and -- overflow in constexpr, evaluated object
size based on alloc_size, << and >> shift count validation, and
OpenMP array section validation.

Diff Detail

Repository
rC Clang

Event Timeline

jdenny created this revision.Mar 22 2019, 12:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2019, 12:47 PM
jdenny set the repository for this revision to rG LLVM Github Monorepo.Mar 22 2019, 12:51 PM
jdenny added a project: Restricted Project.

Might warrant test coverage in llvm/unittest/ADT/APSIntTest.cpp.

Might warrant test coverage in llvm/unittest/ADT/APSIntTest.cpp.

Thanks. I'll work on that.

jdenny updated this revision to Diff 191938.Mar 22 2019, 1:37 PM

Extend llvm/unittests/ADT/APSIntTest.cpp.

Ping. To be clear, my goal here was not to change the OpenMP behavior. My goal was to fix APSInt behavior. If people feel that the old OpenMP behavior is better somehow, we should surely find another way to implement that.

I'm fine with the changes in the OpenMP tests

Thanks. Then the patch just needs someone to review from the ADT side.

lebedev.ri requested changes to this revision.Apr 16 2019, 1:01 PM
lebedev.ri added inline comments.
llvm/unittests/ADT/APSIntTest.cpp
188–194 ↗(On Diff #191938)

I do not understand.
0x7f is 127, it is obviously a maximal positive 8-bit value.
but 0x80 is 128 aka -128, is it not minimal negative 8-bit value?
Is the test correct?

This revision now requires changes to proceed.Apr 16 2019, 1:01 PM
jdenny marked an inline comment as done.Apr 16 2019, 1:22 PM
jdenny added inline comments.
llvm/unittests/ADT/APSIntTest.cpp
188–194 ↗(On Diff #191938)

This test checks that unsigned types are never seen as negative, so it's checking values that would be negative if the type were signed.

lebedev.ri marked an inline comment as done.Apr 16 2019, 1:25 PM
lebedev.ri added inline comments.
llvm/unittests/ADT/APSIntTest.cpp
162 ↗(On Diff #191938)

Can you please duplicate this whole test but for signed APSInt?

188–194 ↗(On Diff #191938)

Ooh, i see. I was looking at the wrong constructor.

explicit APSInt(APInt I, bool isUnsigned = true)
 : APInt(std::move(I)), IsUnsigned(isUnsigned) {}

was the one being called.

Wondering if it would be better to assert for asking for the sign of an unsigned APSInt. I could see a caller just wanting to get the msb for some reason and not knowing that isNegative won’t work.

jdenny updated this revision to Diff 195452.Apr 16 2019, 1:44 PM

Duplicate new APSInt test but for signed type.

jdenny marked 2 inline comments as done.Apr 16 2019, 1:45 PM

Wondering if it would be better to assert for asking for the sign of an unsigned APSInt. I could see a caller just wanting to get the msb for some reason and not knowing that isNegative won’t work.

Yes, i, too, would think an assert is much better solution (since i literally just tripped over this in this review.)

Wondering if it would be better to assert for asking for the sign of an unsigned APSInt. I could see a caller just wanting to get the msb for some reason and not knowing that isNegative won’t work.

Yes, i, too, would think an assert is much better solution (since i literally just tripped over this in this review.)

Imagine a caller wants to check for a negative value for an APSInt from an arbitrary expression. Whether the value is negative is important, but not why. Given that APSInt is documented as knowing its signedness, it seems unintuitive that the caller is responsible for first checking for an unsigned type to avoid an internal compiler error.

If the fear is that developers are too used to calling isNegative to check the high bit, maybe isNegative should always (regardless of signedness) fail an assert for APSInt, and we should find different function names that won't cause such confusion.

Whatever we do for isNegative, we should probably do something similar for isNonNegative as it has the same issues.

Wondering if it would be better to assert for asking for the sign of an unsigned APSInt. I could see a caller just wanting to get the msb for some reason and not knowing that isNegative won’t work.

Yes, i, too, would think an assert is much better solution (since i literally just tripped over this in this review.)

Does this pass check-all? check-all of stage-2? test-suite?

Wondering if it would be better to assert for asking for the sign of an unsigned APSInt. I could see a caller just wanting to get the msb for some reason and not knowing that isNegative won’t work.

Yes, i, too, would think an assert is much better solution (since i literally just tripped over this in this review.)

Does this pass check-all? check-all of stage-2? test-suite?

No. The assert breaks cases in at least ExprConstant.cpp and SemaExpr.cpp.

However, the current version of the patch does not break check-all. I have not tried the others.

Moreover, I see now that this patch fixes behavior for the following:

#include <climits>
#include <iostream>
constexpr unsigned foo() {
  unsigned i = INT_MAX;
  ++i; // should not warn value is outside range
  return i;
}
int main() {
  std::cout << foo() << '\n';
  std::cout << (1 << (unsigned)-1) << '\n'; // should not warn about neg shift count
  return 0;
}

I'll integrate these into the tests later.

Wondering if it would be better to assert for asking for the sign of an unsigned APSInt. I could see a caller just wanting to get the msb for some reason and not knowing that isNegative won’t work.

Yes, i, too, would think an assert is much better solution (since i literally just tripped over this in this review.)

Does this pass check-all? check-all of stage-2? test-suite?

No. The assert breaks cases in at least ExprConstant.cpp and SemaExpr.cpp.

Err, i was talking about the current code in the patch :)

jdenny updated this revision to Diff 195626.Apr 17 2019, 1:46 PM
jdenny edited the summary of this revision. (Show Details)
jdenny added reviewers: rsmith, george.burgess.iv.
jdenny set the repository for this revision to rG LLVM Github Monorepo.

Add tests for various other fixes caused by this patch, update summary, and add related reviewers.

Wondering if it would be better to assert for asking for the sign of an unsigned APSInt. I could see a caller just wanting to get the msb for some reason and not knowing that isNegative won’t work.

Yes, i, too, would think an assert is much better solution (since i literally just tripped over this in this review.)

Does this pass check-all? check-all of stage-2? test-suite?

No. The assert breaks cases in at least ExprConstant.cpp and SemaExpr.cpp.

Err, i was talking about the current code in the patch :)

For me, check-all's success is not affected by the current patch. I built all subprojects except llgo, which gave me a build problem independently of this patch.

I've never tried running the other tests you mention, for any patch. I thought people normally left those to the bots. Should this patch be handled differently?

lebedev.ri resigned from this revision.Apr 18 2019, 3:10 PM

Wondering if it would be better to assert for asking for the sign of an unsigned APSInt. I could see a caller just wanting to get the msb for some reason and not knowing that isNegative won’t work.

Yes, i, too, would think an assert is much better solution (since i literally just tripped over this in this review.)

Does this pass check-all? check-all of stage-2? test-suite?

No. The assert breaks cases in at least ExprConstant.cpp and SemaExpr.cpp.

Err, i was talking about the current code in the patch :)

For me, check-all's success is not affected by the current patch. I built all subprojects except llgo, which gave me a build problem independently of this patch.

(Built natively, or with clang with this patch?)

I've never tried running the other tests you mention, for any patch. I thought people normally left those to the bots. Should this patch be handled differently?

Sounds good.

For me, check-all's success is not affected by the current patch. I built all subprojects except llgo, which gave me a build problem independently of this patch.

(Built natively, or with clang with this patch?)

Built natively.

Wondering if it would be better to assert for asking for the sign of an unsigned APSInt. I could see a caller just wanting to get the msb for some reason and not knowing that isNegative won’t work.

Yes, i, too, would think an assert is much better solution (since i literally just tripped over this in this review.)

Does this pass check-all? check-all of stage-2? test-suite?

No. The assert breaks cases in at least ExprConstant.cpp and SemaExpr.cpp.

Err, i was talking about the current code in the patch :)

For me, check-all's success is not affected by the current patch. I built all subprojects except llgo, which gave me a build problem independently of this patch.

I've never tried running the other tests you mention, for any patch. I thought people normally left those to the bots. Should this patch be handled differently?

We have a lot of people actively working off of trunk, and we try very hard to keep trunk clean. The bots are a second line of defense, not the primary checkers. In any case, this comes down to professional judgement. It is not uncommon to ask for a patch author to check self hosting and a test suite run before committing - specifically, those patches that might affect correctness, or introduce other subtle problems, and for which running the compiler over a bunch of C/C++ code might uncover a problem.

Also, is this review now missing some files? I see here only updates to APSInt.h (only adding functions), APSIntTest.cpp, and a bunch of tests. Nothing that would cause changes to the tests, however (maybe I'm just missing something).

I've never tried running the other tests you mention, for any patch. I thought people normally left those to the bots. Should this patch be handled differently?

We have a lot of people actively working off of trunk, and we try very hard to keep trunk clean. The bots are a second line of defense, not the primary checkers. In any case, this comes down to professional judgement. It is not uncommon to ask for a patch author to check self hosting and a test suite run before committing - specifically, those patches that might affect correctness, or introduce other subtle problems, and for which running the compiler over a bunch of C/C++ code might uncover a problem.

Thanks for explaining. It's my first time receiving these particular requests (probably because of what parts of LLVM I normally edit), so I wasn't sure I understood.

For self-hosting, is it best to build again with CMAKE_C_COMPILER and CMAKE_CXX_COMPILE pointing into the previous build, or is there a better approach?

Also, is this review now missing some files? I see here only updates to APSInt.h (only adding functions), APSIntTest.cpp, and a bunch of tests. Nothing that would cause changes to the tests, however (maybe I'm just missing something).

All looks fine to me. The APSInt.h changes are the reason for all the test changes.

I've never tried running the other tests you mention, for any patch. I thought people normally left those to the bots. Should this patch be handled differently?

We have a lot of people actively working off of trunk, and we try very hard to keep trunk clean. The bots are a second line of defense, not the primary checkers. In any case, this comes down to professional judgement. It is not uncommon to ask for a patch author to check self hosting and a test suite run before committing - specifically, those patches that might affect correctness, or introduce other subtle problems, and for which running the compiler over a bunch of C/C++ code might uncover a problem.

Thanks for explaining. It's my first time receiving these particular requests (probably because of what parts of LLVM I normally edit), so I wasn't sure I understood.

No problem.

For self-hosting, is it best to build again with CMAKE_C_COMPILER and CMAKE_CXX_COMPILE pointing into the previous build, or is there a better approach?

That's what I do.

Also, is this review now missing some files? I see here only updates to APSInt.h (only adding functions), APSIntTest.cpp, and a bunch of tests. Nothing that would cause changes to the tests, however (maybe I'm just missing something).

All looks fine to me. The APSInt.h changes are the reason for all the test changes.

Indeed. I forgot that you were changing overrides.

Thanks!

Does this pass check-all? check-all of stage-2? test-suite?

For me, all these tests behave with the current patch. As before, the only subproject I did not build was llgo.

hfinkel accepted this revision.Apr 19 2019, 6:00 PM

Does this pass check-all? check-all of stage-2? test-suite?

For me, all these tests behave with the current patch. As before, the only subproject I did not build was llgo.

Great. LGTM.

This revision is now accepted and ready to land.Apr 19 2019, 6:00 PM

The APSInt.h itself looks good to me.

Thanks for the reviews! Will push soon.

This revision was automatically updated to reflect the committed changes.