This is an archive of the discontinued LLVM Phabricator instance.

[APSInt] Fix bug in APSInt mentioned in https://github.com/llvm/llvm-project/issues/59515
ClosedPublic

Authored by Peter on Dec 14 2022, 2:42 PM.

Diff Detail

Event Timeline

Peter created this revision.Dec 14 2022, 2:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2022, 2:42 PM
Peter requested review of this revision.Dec 14 2022, 2:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2022, 2:42 PM
Peter retitled this revision from [ADT] Fix bug in APSInt mentioned in https://github.com/llvm/llvm-project/issues/59515 to [APSInt] Fix bug in APSInt mentioned in https://github.com/llvm/llvm-project/issues/59515.Dec 14 2022, 2:43 PM
Peter added reviewers: RKSimon, efriedma.
Peter updated this revision to Diff 483002.Dec 14 2022, 2:45 PM

revert the format.

Original APSInt.h doesn't comply with clang-format. Maybe change it in another patch

RKSimon added inline comments.Dec 16 2022, 6:52 AM
llvm/unittests/ADT/APSIntTest.cpp
88

why can't a i128 -7 be represented as a i64 -7?

Peter updated this revision to Diff 483635.Dec 16 2022, 12:13 PM

add a unit test

Peter marked an inline comment as done.Dec 16 2022, 12:15 PM
Peter added inline comments.
llvm/unittests/ADT/APSIntTest.cpp
88

APInt(128, -7) == (uint64)-7, there are 64 leading zeros and thus is a actually a huge positive number.

I added two more tests to make this clear.

barannikov88 added inline comments.
llvm/include/llvm/ADT/APSInt.h
94

The comment near getMinSignBits says that it is deprecated (and "use getSignificantBits() instead").
There are also isSignedIntN and isIntN, which can be useful:

return isSigned() ? isSignedIntN(64) : isIntN(63);
Peter updated this revision to Diff 483662.Dec 16 2022, 1:48 PM
Peter marked an inline comment as done.

using another API for isRepresentableByInt64

RKSimon accepted this revision.Dec 17 2022, 2:04 AM

LGTM

This revision is now accepted and ready to land.Dec 17 2022, 2:04 AM
Peter updated this revision to Diff 483775.Dec 17 2022, 1:08 PM

diff against formatted version

tschuett added inline comments.
llvm/include/llvm/ADT/APSInt.h
104

You don't need the constructor. getExtValue()should work.

Peter marked 2 inline comments as done.Dec 17 2022, 1:23 PM
Peter added inline comments.
llvm/include/llvm/ADT/APSInt.h
104

Yes I do, otherwise the static type of the two branch don't match, as discussed here: https://reviews.llvm.org/D139683

llvm/include/llvm/ADT/APInt.h:1500:36: error: incompatible operand types ('uint64_t' (aka 'unsigned long') and 'const std::nullopt_t')
    return (getActiveBits() <= 64) ? getZExtValue() : std::nullopt;
                                   ^ ~~~~~~~~~~~~~~   ~~~~~~~~~~~~
Peter updated this revision to Diff 483790.Dec 17 2022, 5:01 PM
Peter marked an inline comment as done.

rebase to workable version

Peter added a comment.Dec 17 2022, 5:09 PM

NP. I didn't know that either until I was asked to do the same in that patch :)

This revision was landed with ongoing or failed builds.Dec 18 2022, 1:36 PM
This revision was automatically updated to reflect the committed changes.
Peter reopened this revision.Dec 23 2022, 11:11 AM

Reopen to fix bugs caused by incorrect use of getExtValue()

This revision is now accepted and ready to land.Dec 23 2022, 11:11 AM
Peter updated this revision to Diff 485149.Dec 23 2022, 11:11 AM

fix a bug caused by incorrect use of getExtValue

RKSimon accepted this revision.Dec 24 2022, 4:01 AM

LGTM

Peter added a reviewer: pcc.Dec 26 2022, 4:32 PM
Peter updated this revision to Diff 485327.Dec 26 2022, 6:06 PM

Update clang

Herald added a project: Restricted Project. · View Herald TranscriptDec 26 2022, 6:06 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Peter updated this revision to Diff 485328.Dec 26 2022, 6:06 PM

change base

Peter added a comment.Dec 26 2022, 6:08 PM

After this patch the semantics of getExtValue changed and thus broke clang. @pcc Would you mind take a look if the modification works for clang.

efriedma added inline comments.Jan 4 2023, 10:53 AM
clang/lib/CodeGen/CGDebugInfo.cpp
5386 ↗(On Diff #485328)

I think we actually want the existing behavior here. Values embedded in debug info aren't really signed or unsigned; they're interpreted by the debugger based on the type of the value.

Maybe it makes sense to add a new APSInt API for that? Or I guess we could explicitly write out Init.getInt().isSigned() ? Init.getInt().getSExtValue() : Init.getInt().getZExtValue(); if we can't think of a reasonable name for the new API...

Peter updated this revision to Diff 487118.Jan 7 2023, 12:22 PM

[DebugInfo] update clang to match semantics changes in APSInt

Peter marked an inline comment as done.Jan 7 2023, 12:22 PM
Peter added inline comments.
clang/lib/CodeGen/CGDebugInfo.cpp
5386 ↗(On Diff #485328)

I have no problem with this proposed new API, we may even name it uint64_t tryInt64Representaiton. My main concern is that returning a value whose static type is unsigned while semantics can be signed OR unsigned is a bit inconsist.

I will use something like Init.getInt().isSigned() ? Init.getInt().getSExtValue() : Init.getInt().getZExtValue(); for now.

Peter marked an inline comment as done.Jan 10 2023, 11:02 AM

@pcc @efriedma Would you mind review the changes to clang when you are free? If they look good I'll merge them.

Peter updated this revision to Diff 487946.Jan 10 2023, 12:18 PM

use const ref

Hello, it appears this patch causes a crash when I analyze this reproducer, with Z3 enabled. In the case shown here, the analyzer finds that 'f' to the call a(f) is uninitialized, and then is attempted to be refuted through SMTConv, leading to the assertion.

If I modify the below code to not use isRepresentableByIn64(), or use 'assert(getMinSignedBits() <= 64 && "Too many bits for int64_t");' instead, I do not see the crash.

clang --analyze -Xclang -analyzer-config -Xclang crosscheck-with-z3=true --target=x86_64-redhat-linux case.c

void a(int);
typedef struct {
  int b;
} c;
c *d;
void e() {
  (void)d->b;
  int f;
  a(f);
}

The assert ...

clang-16: ../include/llvm/ADT/APSInt.h:99: int64_t llvm::APSInt::getExtValue() const: Assertion `isRepresentableByInt64() && "Too many bits for int64_t"' failed.Program received signal SIGABRT, Aborted.
Peter added a comment.Jan 25 2023, 8:44 PM

Hello, it appears this patch causes a crash when I analyze this reproducer, with Z3 enabled. In the case shown here, the analyzer finds that 'f' to the call a(f) is uninitialized, and then is attempted to be refuted through SMTConv, leading to the assertion.

If I modify the below code to not use isRepresentableByIn64(), or use 'assert(getMinSignedBits() <= 64 && "Too many bits for int64_t");' instead, I do not see the crash.

clang --analyze -Xclang -analyzer-config -Xclang crosscheck-with-z3=true --target=x86_64-redhat-linux case.c

void a(int);
typedef struct {
  int b;
} c;
c *d;
void e() {
  (void)d->b;
  int f;
  a(f);
}

The assert ...

clang-16: ../include/llvm/ADT/APSInt.h:99: int64_t llvm::APSInt::getExtValue() const: Assertion `isRepresentableByInt64() && "Too many bits for int64_t"' failed.Program received signal SIGABRT, Aborted.

Thanks for helping identifying the bug.

This is not a problem in this patch, the semantics of getExtValue changed in this patch as carefully discussed here.
To summarize, we believe its not reasonable to carry out ext for (signed)(int64::Max + 1). Although it's still 64 bit, the returned value would be interpreted as int64::Min instead of a large positive integer. This behavior was allowed before.

To provide a fix for this problem, this patch provides a better way to determine if the extension is possible isRepresentableByInt64, I would recommend replacing if (LLVM_UNLIKELY(Int.getBitWidth() > 64u)) in Z3Solver.cpp:732 with if (LLVM_UNLIKELY(!Int.isRepresentableByInt64())).

vabridgers added a comment.EditedJan 26 2023, 2:16 AM

Thanks @Peter. I tried that, and the crash is resolved. Submitted https://reviews.llvm.org/D142627 for review.