[llvm] r253497 - [llvm-profdata] Add SaturatingAdd/SaturatingMultiply Helper Functions (2nd try)
Bruno Cardoso Lopes via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 18 16:28:22 PST 2015
Hi Nathan,
Ubsan is detecting some undefined behavior after your commit:
http://lab.llvm.org:8080/green/job/clang-stage2-cmake-RgSan_check/746/
******************** TEST 'LLVM-Unit ::
Support/SupportTests/MathExtras.SaturatingMultiply' FAILED
********************
Note: Google Test filter = MathExtras.SaturatingMultiply
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from MathExtras
[ RUN ] MathExtras.SaturatingMultiply
/Users/buildslave/jenkins/sharedspace/clang-stage2-cmake-RgSan at 2/llvm/include/llvm/Support/MathExtras.h:675:11:
runtime error: signed integer overflow: 65535 * 65535 cannot be
represented in type 'int'
SUMMARY: AddressSanitizer: undefined-behavior
/Users/buildslave/jenkins/sharedspace/clang-stage2-cmake-RgSan at 2/llvm/include/llvm/Support/MathExtras.h:675:11
in
0 SupportTests 0x0000000105b92545
llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 277
1 SupportTests 0x0000000105b90400
llvm::sys::RunSignalHandlers() + 448
2 SupportTests 0x0000000105b937d3
SignalHandler(int) + 611
3 libsystem_platform.dylib 0x00007fff8d239f1a _sigtramp + 26
4 libsystem_platform.dylib 0x0000000000000003 _sigtramp + 1927045379
5 libsystem_c.dylib 0x00007fff88edd9b3 abort + 129
6 libclang_rt.asan_osx_dynamic.dylib 0x00000001060c0f76
__sanitizer::Abort() + 6
On Wed, Nov 18, 2015 at 12:40 PM, Nathan Slingerland via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
> Author: slingn
> Date: Wed Nov 18 14:40:41 2015
> New Revision: 253497
>
> URL: http://llvm.org/viewvc/llvm-project?rev=253497&view=rev
> Log:
> [llvm-profdata] Add SaturatingAdd/SaturatingMultiply Helper Functions (2nd try)
>
> Summary:
> This change adds MathExtras helper functions for handling unsigned, saturating addition and multiplication. It also updates the instrumentation and sample profile merge implementations to use them.
>
> Reviewers: dnovillo, bogner, davidxl
>
> Subscribers: davidxl, llvm-commits
>
> Differential Revision: http://reviews.llvm.org/D14720
>
> Modified:
> llvm/trunk/include/llvm/ProfileData/InstrProf.h
> llvm/trunk/include/llvm/ProfileData/SampleProf.h
> llvm/trunk/include/llvm/Support/MathExtras.h
> llvm/trunk/unittests/ProfileData/InstrProfTest.cpp
> llvm/trunk/unittests/Support/MathExtrasTest.cpp
>
> Modified: llvm/trunk/include/llvm/ProfileData/InstrProf.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProf.h?rev=253497&r1=253496&r2=253497&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/ProfileData/InstrProf.h (original)
> +++ llvm/trunk/include/llvm/ProfileData/InstrProf.h Wed Nov 18 14:40:41 2015
> @@ -226,7 +226,7 @@ struct InstrProfValueSiteRecord {
> while (I != IE && I->Value < J->Value)
> ++I;
> if (I != IE && I->Value == J->Value) {
> - I->Count += J->Count;
> + I->Count = SaturatingAdd(I->Count, J->Count);
> ++I;
> continue;
> }
>
> Modified: llvm/trunk/include/llvm/ProfileData/SampleProf.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/SampleProf.h?rev=253497&r1=253496&r2=253497&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/ProfileData/SampleProf.h (original)
> +++ llvm/trunk/include/llvm/ProfileData/SampleProf.h Wed Nov 18 14:40:41 2015
> @@ -173,10 +173,7 @@ public:
> /// Sample counts accumulate using saturating arithmetic, to avoid wrapping
> /// around unsigned integers.
> void addSamples(uint64_t S) {
> - if (NumSamples <= std::numeric_limits<uint64_t>::max() - S)
> - NumSamples += S;
> - else
> - NumSamples = std::numeric_limits<uint64_t>::max();
> + NumSamples = SaturatingAdd(NumSamples, S);
> }
>
> /// Add called function \p F with samples \p S.
> @@ -185,10 +182,7 @@ public:
> /// around unsigned integers.
> void addCalledTarget(StringRef F, uint64_t S) {
> uint64_t &TargetSamples = CallTargets[F];
> - if (TargetSamples <= std::numeric_limits<uint64_t>::max() - S)
> - TargetSamples += S;
> - else
> - TargetSamples = std::numeric_limits<uint64_t>::max();
> + TargetSamples = SaturatingAdd(TargetSamples, S);
> }
>
> /// Return true if this sample record contains function calls.
>
> Modified: llvm/trunk/include/llvm/Support/MathExtras.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/MathExtras.h?rev=253497&r1=253496&r2=253497&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Support/MathExtras.h (original)
> +++ llvm/trunk/include/llvm/Support/MathExtras.h Wed Nov 18 14:40:41 2015
> @@ -653,6 +653,32 @@ inline int64_t SignExtend64(uint64_t X,
> return int64_t(X << (64 - B)) >> (64 - B);
> }
>
> +/// \brief Add two unsigned integers, X and Y, of type T.
> +/// Clamp the result to the maximum representable value of T on overflow.
> +template <typename T>
> +typename std::enable_if<std::is_unsigned<T>::value, T>::type
> +SaturatingAdd(T X, T Y) {
> + // Hacker's Delight, p. 29
> + T Z = X + Y;
> + if (Z < X || Z < Y)
> + return std::numeric_limits<T>::max();
> + else
> + return Z;
> +}
> +
> +/// \brief Multiply two unsigned integers, X and Y, of type T.
> +/// Clamp the result to the maximum representable value of T on overflow.
> +template <typename T>
> +typename std::enable_if<std::is_unsigned<T>::value, T>::type
> +SaturatingMultiply(T X, T Y) {
> + // Hacker's Delight, p. 30
> + T Z = X * Y;
> + if (Y != 0 && Z / Y != X)
> + return std::numeric_limits<T>::max();
> + else
> + return Z;
> +}
> +
> extern const float huge_valf;
> } // End llvm namespace
>
>
> Modified: llvm/trunk/unittests/ProfileData/InstrProfTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ProfileData/InstrProfTest.cpp?rev=253497&r1=253496&r2=253497&view=diff
> ==============================================================================
> --- llvm/trunk/unittests/ProfileData/InstrProfTest.cpp (original)
> +++ llvm/trunk/unittests/ProfileData/InstrProfTest.cpp Wed Nov 18 14:40:41 2015
> @@ -350,6 +350,38 @@ TEST_F(InstrProfTest, get_icall_data_mer
> ASSERT_EQ(2U, VD_4[2].Count);
> }
>
> +TEST_F(InstrProfTest, get_icall_data_merge1_saturation) {
> + const uint64_t Max = std::numeric_limits<uint64_t>::max();
> +
> + InstrProfRecord Record1("caller", 0x1234, {1});
> + InstrProfRecord Record2("caller", 0x1234, {1});
> + InstrProfRecord Record3("callee1", 0x1235, {3, 4});
> +
> + Record1.reserveSites(IPVK_IndirectCallTarget, 1);
> + InstrProfValueData VD1[] = {{(uint64_t) "callee1", 1}};
> + Record1.addValueData(IPVK_IndirectCallTarget, 0, VD1, 1, nullptr);
> +
> + Record2.reserveSites(IPVK_IndirectCallTarget, 1);
> + InstrProfValueData VD2[] = {{(uint64_t) "callee1", Max}};
> + Record2.addValueData(IPVK_IndirectCallTarget, 0, VD2, 1, nullptr);
> +
> + Writer.addRecord(std::move(Record1));
> + Writer.addRecord(std::move(Record2));
> + Writer.addRecord(std::move(Record3));
> +
> + auto Profile = Writer.writeBuffer();
> + readProfile(std::move(Profile));
> +
> + // Verify saturation of counts.
> + ErrorOr<InstrProfRecord> R = Reader->getInstrProfRecord("caller", 0x1234);
> + ASSERT_TRUE(NoError(R.getError()));
> + ASSERT_EQ(1U, R.get().getNumValueSites(IPVK_IndirectCallTarget));
> + std::unique_ptr<InstrProfValueData[]> VD =
> + R.get().getValueForSite(IPVK_IndirectCallTarget, 0);
> + ASSERT_EQ(StringRef("callee1"), StringRef((const char *)VD[0].Value, 7));
> + ASSERT_EQ(Max, VD[0].Count);
> +}
> +
> TEST_F(InstrProfTest, get_max_function_count) {
> InstrProfRecord Record1("foo", 0x1234, {1ULL << 31, 2});
> InstrProfRecord Record2("bar", 0, {1ULL << 63});
>
> Modified: llvm/trunk/unittests/Support/MathExtrasTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/MathExtrasTest.cpp?rev=253497&r1=253496&r2=253497&view=diff
> ==============================================================================
> --- llvm/trunk/unittests/Support/MathExtrasTest.cpp (original)
> +++ llvm/trunk/unittests/Support/MathExtrasTest.cpp Wed Nov 18 14:40:41 2015
> @@ -190,4 +190,40 @@ TEST(MathExtras, RoundUpToAlignment) {
> EXPECT_EQ(552u, RoundUpToAlignment(321, 255, 42));
> }
>
> +template<typename T>
> +void SaturatingAddTestHelper()
> +{
> + const T Max = std::numeric_limits<T>::max();
> + EXPECT_EQ(T(3), SaturatingAdd(T(1), T(2)));
> + EXPECT_EQ(Max, SaturatingAdd(Max, T(1)));
> + EXPECT_EQ(Max, SaturatingAdd(T(1), Max));
> + EXPECT_EQ(Max, SaturatingAdd(Max, Max));
> +}
> +
> +TEST(MathExtras, SaturatingAdd) {
> + SaturatingAddTestHelper<uint8_t>();
> + SaturatingAddTestHelper<uint16_t>();
> + SaturatingAddTestHelper<uint32_t>();
> + SaturatingAddTestHelper<uint64_t>();
> +}
> +
> +template<typename T>
> +void SaturatingMultiplyTestHelper()
> +{
> + const T Max = std::numeric_limits<T>::max();
> + EXPECT_EQ(T(0), SaturatingMultiply(T(1), T(0)));
> + EXPECT_EQ(T(0), SaturatingMultiply(T(0), T(1)));
> + EXPECT_EQ(T(6), SaturatingMultiply(T(2), T(3)));
> + EXPECT_EQ(Max, SaturatingMultiply(Max, T(2)));
> + EXPECT_EQ(Max, SaturatingMultiply(T(2),Max));
> + EXPECT_EQ(Max, SaturatingMultiply(Max, Max));
> +}
> +
> +TEST(MathExtras, SaturatingMultiply) {
> + SaturatingMultiplyTestHelper<uint8_t>();
> + SaturatingMultiplyTestHelper<uint16_t>();
> + SaturatingMultiplyTestHelper<uint32_t>();
> + SaturatingMultiplyTestHelper<uint64_t>();
> +}
> +
> }
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
--
Bruno Cardoso Lopes
http://www.brunocardoso.cc
More information about the llvm-commits
mailing list