This is an archive of the discontinued LLVM Phabricator instance.

[APInt] provide a safe API for zext value and sext value.
ClosedPublic

Authored by Peter on Dec 8 2022, 4:59 PM.

Details

Summary

Currently, APInt::getSExtValue and getZExtValue crashes on values with more than 64 bits.
Users may accidently crash the compiler with this setting when the integer may be i128.
As shown in https://github.com/llvm/llvm-project/issues/59316

In this patch we provide a trySExtValue and tryZExtValue to return an Optional, the user
needs to explictly unwrap it and condsier the possibility where there my no value in it.

Diff Detail

Event Timeline

Peter created this revision.Dec 8 2022, 4:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2022, 4:59 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
Peter requested review of this revision.Dec 8 2022, 4:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2022, 4:59 PM
lebedev.ri added inline comments.
llvm/lib/Support/APInt.cpp
506–518 ↗(On Diff #481476)

These should just check for precondition and call the normal functions

Peter updated this revision to Diff 481482.Dec 8 2022, 5:05 PM

use existing API

Peter marked an inline comment as done.Dec 8 2022, 5:05 PM
Peter added inline comments.
llvm/lib/Support/APInt.cpp
506–518 ↗(On Diff #481476)

You are right, thanks for pointing that out.

RKSimon added inline comments.Dec 9 2022, 12:08 AM
llvm/include/llvm/ADT/APInt.h
1499

std::optional ?

llvm/lib/Support/APInt.cpp
514 ↗(On Diff #481482)

inline?

Peter updated this revision to Diff 481800.Dec 9 2022, 5:21 PM
Peter marked an inline comment as done.

use std::optional and inlining the API

RKSimon added inline comments.Dec 11 2022, 2:32 AM
llvm/include/llvm/ADT/APInt.h
1522

getSignificantBits - you might need to alter the unit test as 'all bits' representations will safely truncate to -1

tschuett added inline comments.
llvm/include/llvm/ADT/APInt.h
1501

std::nullopt?

RKSimon added inline comments.Dec 11 2022, 6:08 AM
llvm/include/llvm/ADT/APInt.h
1501

+1

tschuett added inline comments.Dec 11 2022, 6:18 AM
llvm/unittests/ADT/APIntTest.cpp
3136

This is opinion and not review!!! For unit tests, I prefer .has_value() instead of the boolean conversion to make the intent clear.

Peter updated this revision to Diff 481990.Dec 11 2022, 8:20 PM
Peter marked 5 inline comments as done.

use std::nullopt, add unit test

RKSimon added inline comments.Dec 12 2022, 1:26 AM
llvm/include/llvm/ADT/APInt.h
1518

zero -> sign

1522

getActiveBits ->getSignificantBits

tschuett added inline comments.Dec 12 2022, 1:30 AM
llvm/include/llvm/ADT/APInt.h
1501

std::optional<uint64_t>(std::nullopt). --> std::nullopt .

std::optional<uint64_t>(getZExtValue()) ->. getZExtValue().

Also, please can you add APSInt::tryExtValue in this patch as well?

Peter updated this revision to Diff 482227.Dec 12 2022, 12:24 PM
Peter marked 3 inline comments as done.

minor changes, adding tryExt to APSInt.

Peter added inline comments.Dec 12 2022, 12:28 PM
llvm/unittests/ADT/APSIntTest.cpp
94

This test is explicitly commented out. It will trigger a bug in getExtValue. Do we want to fix it in this patch or another?

Bug description: There is an inconsistency in getExtValue. When checking if the operation is possible, it is always treated as a signed integer (getMinSignedBits), but when casting, it may be treated as an unsigned.

This inconsistency will crash all unsigned integer > 64 bits, with all high bits set to 1. (e.g. here its u128_max). getExtValue will determine the cast to unsigned int64 is possible(because it's treated as -1) but when casting getZExtValue will find it impossible.

Oh - that's annoying! Let's drop the APSInt changes and do them as follow ups.

Peter added a comment.Dec 13 2022, 8:50 PM

Oh - that's annoying! Let's drop the APSInt changes and do them as follow ups.

Will do

Peter updated this revision to Diff 482704.Dec 13 2022, 8:53 PM

remove APSInt change. will patch it in the future

RKSimon added inline comments.Dec 14 2022, 1:00 AM
llvm/include/llvm/ADT/APInt.h
1522

Still needs to be converted to getSignificantBits to match getSExtValue

I've raised https://github.com/llvm/llvm-project/issues/59515 to cover the APSInt::getExtValue() issue

Peter updated this revision to Diff 482982.Dec 14 2022, 1:47 PM
Peter marked an inline comment as done.

update signed cast

llvm/include/llvm/ADT/APInt.h
1522

Sry I forgot about it. This patch should be good to go.

RKSimon accepted this revision.Dec 15 2022, 12:57 AM

LGTM with a couple of minors - cheers!

llvm/include/llvm/ADT/APInt.h
1501

Any reason you can't remove the std::optional<uint64_t>() cast?

1522

Any reason you can't remove the std::optional<int64_t>() cast?

This revision is now accepted and ready to land.Dec 15 2022, 12:57 AM
Peter added inline comments.Dec 15 2022, 9:56 AM
llvm/include/llvm/ADT/APInt.h
1501

Err if I remove both cast, the static type of two branches won't match. Seems implicit cast didn't kick in.

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;
                                   ^ ~~~~~~~~~~~~~~   ~~~~~~~~~~~~

Do you know any work arounds?

Peter added inline comments.Dec 15 2022, 9:58 AM
llvm/include/llvm/ADT/APInt.h
1501

Oh, If I rewrite this as an if-branch that works:

if (getActiveBits() <= 64)
  return getZExtValue();
return std::nullopt;

Which one would you prefer?

RKSimon added inline comments.Dec 15 2022, 10:01 AM
llvm/include/llvm/ADT/APInt.h
1501

Ah - of course! Keeping as it is is fine - thanks!

This revision was landed with ongoing or failed builds.Dec 15 2022, 10:06 AM
This revision was automatically updated to reflect the committed changes.