[llvm] r253497 - [llvm-profdata] Add SaturatingAdd/SaturatingMultiply Helper Functions (2nd try)
Richard Smith via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 19 18:19:40 PST 2015
The code is indeed broken, as ubsan reports (see my response to that bug).
What do you think of the attached alternative implementation? It should fix
the signed integer overflow bug and is probably faster in all cases too
(since it doesn't use a division to check for overflow).
On Thu, Nov 19, 2015 at 3:53 PM, Nathan Slingerland via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> I've filed the issue as https://llvm.org/bugs/show_bug.cgi?id=25580
>
> I'd still suggest we move forward with the MathExtras.h patch since that
> unblocks the unit tests and is no more expensive than the previous
> implementation.
>
> On Thu, Nov 19, 2015 at 3:47 PM, Xinliang David Li <xinliangli at gmail.com>
> wrote:
>
>> 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
>>>>
>>>>
>>>
>>
>
> _______________________________________________
> 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/69207e56/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-sat-mul-ub.patch
Type: application/octet-stream
Size: 1444 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151119/69207e56/attachment.obj>
More information about the llvm-commits
mailing list