[llvm] r253497 - [llvm-profdata] Add SaturatingAdd/SaturatingMultiply Helper Functions (2nd try)
Xinliang David Li via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 19 15:47:08 PST 2015
It might be better to extract a small reproducible and report to sanitizer
folks. If it is a bug there -- we should fix it there.
David
On Thu, Nov 19, 2015 at 3:01 PM, Nathan Slingerland <slingn at gmail.com>
wrote:
> I am able to reproduce the failure using -fsanitizer=undefined (I tried on
> Ubuntu 15.10 x86_64, clang 3.6).
>
> Test case: http://pastebin.com/Eejy5rNB
>
> $ clang++ -O1 -std=c++11 -fsanitize=undefined test.cpp -o test
> $ ./test
> main.cpp:25:11: runtime error: signed integer overflow: 65535 * 65535
> cannot be represented in type 'int'
>
> Interestingly, the failure only occurs with uint16_t.
> The other unsigned types (uint8_t/uint32_t/uint64_t) pass if I comment out
> the uint16_t case.
> It's not clear to me why this is triggering the signed multiply check with
> uint16_t.
>
> I have put a fix (work-around, really) up for review as D14845.
>
>
> On Wed, Nov 18, 2015 at 9:48 PM, Xinliang David Li via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> probably
>>
>> cmake -DLLVM_USE_SANITIZER=Undefined ...
>>
>> David
>>
>> On Wed, Nov 18, 2015 at 9:37 PM, Sean Silva via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> What is the configure line for this bot?
>>>
>>> On Wed, Nov 18, 2015 at 4:28 PM, Bruno Cardoso Lopes via llvm-commits <
>>> llvm-commits at lists.llvm.org> wrote:
>>>
>>>> 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
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
>>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151119/87e829b1/attachment.html>
More information about the llvm-commits
mailing list