<div dir="ltr">The code is indeed broken, as ubsan reports (see my response to that bug).<div><br></div><div>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).</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Nov 19, 2015 at 3:53 PM, Nathan Slingerland via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>I've filed the issue as <a href="https://llvm.org/bugs/show_bug.cgi?id=25580" target="_blank">https://llvm.org/bugs/show_bug.cgi?id=25580</a><br><br></div>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.</div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Nov 19, 2015 at 3:47 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:xinliangli@gmail.com" target="_blank">xinliangli@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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.<span><font color="#888888"><div><br></div><div>David</div></font></span></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Nov 19, 2015 at 3:01 PM, Nathan Slingerland <span dir="ltr"><<a href="mailto:slingn@gmail.com" target="_blank">slingn@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div><div><div><div>I am able to reproduce the failure using -fsanitizer=undefined (I tried on Ubuntu 15.10 x86_64, clang 3.6).<br><br></div>Test case: <a href="http://pastebin.com/Eejy5rNB" target="_blank">http://pastebin.com/Eejy5rNB</a><br><br>$ clang++ -O1 -std=c++11 -fsanitize=undefined test.cpp -o test<br>$ ./test<br>main.cpp:25:11: runtime error: signed integer overflow: 65535 * 65535 cannot be represented in type 'int'<br><br></div>Interestingly, the failure only occurs with uint16_t.<br>The other unsigned types (uint8_t/uint32_t/uint64_t) pass if I comment out the uint16_t case.<br></div>It's not clear to me why this is triggering the signed multiply check with uint16_t.<br></div><br></div>I have put a fix (work-around, really) up for review as D14845.<br><div><div><div><div><div><div><div><br></div></div></div></div></div></div></div></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 18, 2015 at 9:48 PM, Xinliang David Li via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">probably<div><br></div><div>cmake -DLLVM_USE_SANITIZER=Undefined ...</div><div><br></div><div>David</div></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 18, 2015 at 9:37 PM, Sean Silva via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">What is the configure line for this bot?</div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 18, 2015 at 4:28 PM, Bruno Cardoso Lopes via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Nathan,<br>
<br>
Ubsan is detecting some undefined behavior after your commit:<br>
<a href="http://lab.llvm.org:8080/green/job/clang-stage2-cmake-RgSan_check/746/" rel="noreferrer" target="_blank">http://lab.llvm.org:8080/green/job/clang-stage2-cmake-RgSan_check/746/</a><br>
<br>
******************** TEST 'LLVM-Unit ::<br>
Support/SupportTests/MathExtras.SaturatingMultiply' FAILED<br>
********************<br>
Note: Google Test filter = MathExtras.SaturatingMultiply<br>
[==========] Running 1 test from 1 test case.<br>
[----------] Global test environment set-up.<br>
[----------] 1 test from MathExtras<br>
[ RUN ] MathExtras.SaturatingMultiply<br>
/Users/buildslave/jenkins/sharedspace/clang-stage2-cmake-RgSan@2/llvm/include/llvm/Support/MathExtras.h:675:11:<br>
runtime error: signed integer overflow: 65535 * 65535 cannot be<br>
represented in type 'int'<br>
SUMMARY: AddressSanitizer: undefined-behavior<br>
/Users/buildslave/jenkins/sharedspace/clang-stage2-cmake-RgSan@2/llvm/include/llvm/Support/MathExtras.h:675:11<br>
in<br>
0 SupportTests 0x0000000105b92545<br>
llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 277<br>
1 SupportTests 0x0000000105b90400<br>
llvm::sys::RunSignalHandlers() + 448<br>
2 SupportTests 0x0000000105b937d3<br>
SignalHandler(int) + 611<br>
3 libsystem_platform.dylib 0x00007fff8d239f1a _sigtramp + 26<br>
4 libsystem_platform.dylib 0x0000000000000003 _sigtramp + 1927045379<br>
5 libsystem_c.dylib 0x00007fff88edd9b3 abort + 129<br>
6 libclang_rt.asan_osx_dynamic.dylib 0x00000001060c0f76<br>
__sanitizer::Abort() + 6<br>
<div><div><br>
<br>
<br>
On Wed, Nov 18, 2015 at 12:40 PM, Nathan Slingerland via llvm-commits<br>
<<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
> Author: slingn<br>
> Date: Wed Nov 18 14:40:41 2015<br>
> New Revision: 253497<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=253497&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=253497&view=rev</a><br>
> Log:<br>
> [llvm-profdata] Add SaturatingAdd/SaturatingMultiply Helper Functions (2nd try)<br>
><br>
> Summary:<br>
> 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.<br>
><br>
> Reviewers: dnovillo, bogner, davidxl<br>
><br>
> Subscribers: davidxl, llvm-commits<br>
><br>
> Differential Revision: <a href="http://reviews.llvm.org/D14720" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14720</a><br>
><br>
> Modified:<br>
> llvm/trunk/include/llvm/ProfileData/InstrProf.h<br>
> llvm/trunk/include/llvm/ProfileData/SampleProf.h<br>
> llvm/trunk/include/llvm/Support/MathExtras.h<br>
> llvm/trunk/unittests/ProfileData/InstrProfTest.cpp<br>
> llvm/trunk/unittests/Support/MathExtrasTest.cpp<br>
><br>
> Modified: llvm/trunk/include/llvm/ProfileData/InstrProf.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProf.h?rev=253497&r1=253496&r2=253497&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProf.h?rev=253497&r1=253496&r2=253497&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/include/llvm/ProfileData/InstrProf.h (original)<br>
> +++ llvm/trunk/include/llvm/ProfileData/InstrProf.h Wed Nov 18 14:40:41 2015<br>
> @@ -226,7 +226,7 @@ struct InstrProfValueSiteRecord {<br>
> while (I != IE && I->Value < J->Value)<br>
> ++I;<br>
> if (I != IE && I->Value == J->Value) {<br>
> - I->Count += J->Count;<br>
> + I->Count = SaturatingAdd(I->Count, J->Count);<br>
> ++I;<br>
> continue;<br>
> }<br>
><br>
> Modified: llvm/trunk/include/llvm/ProfileData/SampleProf.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/SampleProf.h?rev=253497&r1=253496&r2=253497&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/SampleProf.h?rev=253497&r1=253496&r2=253497&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/include/llvm/ProfileData/SampleProf.h (original)<br>
> +++ llvm/trunk/include/llvm/ProfileData/SampleProf.h Wed Nov 18 14:40:41 2015<br>
> @@ -173,10 +173,7 @@ public:<br>
> /// Sample counts accumulate using saturating arithmetic, to avoid wrapping<br>
> /// around unsigned integers.<br>
> void addSamples(uint64_t S) {<br>
> - if (NumSamples <= std::numeric_limits<uint64_t>::max() - S)<br>
> - NumSamples += S;<br>
> - else<br>
> - NumSamples = std::numeric_limits<uint64_t>::max();<br>
> + NumSamples = SaturatingAdd(NumSamples, S);<br>
> }<br>
><br>
> /// Add called function \p F with samples \p S.<br>
> @@ -185,10 +182,7 @@ public:<br>
> /// around unsigned integers.<br>
> void addCalledTarget(StringRef F, uint64_t S) {<br>
> uint64_t &TargetSamples = CallTargets[F];<br>
> - if (TargetSamples <= std::numeric_limits<uint64_t>::max() - S)<br>
> - TargetSamples += S;<br>
> - else<br>
> - TargetSamples = std::numeric_limits<uint64_t>::max();<br>
> + TargetSamples = SaturatingAdd(TargetSamples, S);<br>
> }<br>
><br>
> /// Return true if this sample record contains function calls.<br>
><br>
> Modified: llvm/trunk/include/llvm/Support/MathExtras.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/MathExtras.h?rev=253497&r1=253496&r2=253497&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/MathExtras.h?rev=253497&r1=253496&r2=253497&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/include/llvm/Support/MathExtras.h (original)<br>
> +++ llvm/trunk/include/llvm/Support/MathExtras.h Wed Nov 18 14:40:41 2015<br>
> @@ -653,6 +653,32 @@ inline int64_t SignExtend64(uint64_t X,<br>
> return int64_t(X << (64 - B)) >> (64 - B);<br>
> }<br>
><br>
> +/// \brief Add two unsigned integers, X and Y, of type T.<br>
> +/// Clamp the result to the maximum representable value of T on overflow.<br>
> +template <typename T><br>
> +typename std::enable_if<std::is_unsigned<T>::value, T>::type<br>
> +SaturatingAdd(T X, T Y) {<br>
> + // Hacker's Delight, p. 29<br>
> + T Z = X + Y;<br>
> + if (Z < X || Z < Y)<br>
> + return std::numeric_limits<T>::max();<br>
> + else<br>
> + return Z;<br>
> +}<br>
> +<br>
> +/// \brief Multiply two unsigned integers, X and Y, of type T.<br>
> +/// Clamp the result to the maximum representable value of T on overflow.<br>
> +template <typename T><br>
> +typename std::enable_if<std::is_unsigned<T>::value, T>::type<br>
> +SaturatingMultiply(T X, T Y) {<br>
> + // Hacker's Delight, p. 30<br>
> + T Z = X * Y;<br>
> + if (Y != 0 && Z / Y != X)<br>
> + return std::numeric_limits<T>::max();<br>
> + else<br>
> + return Z;<br>
> +}<br>
> +<br>
> extern const float huge_valf;<br>
> } // End llvm namespace<br>
><br>
><br>
> Modified: llvm/trunk/unittests/ProfileData/InstrProfTest.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ProfileData/InstrProfTest.cpp?rev=253497&r1=253496&r2=253497&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ProfileData/InstrProfTest.cpp?rev=253497&r1=253496&r2=253497&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/unittests/ProfileData/InstrProfTest.cpp (original)<br>
> +++ llvm/trunk/unittests/ProfileData/InstrProfTest.cpp Wed Nov 18 14:40:41 2015<br>
> @@ -350,6 +350,38 @@ TEST_F(InstrProfTest, get_icall_data_mer<br>
> ASSERT_EQ(2U, VD_4[2].Count);<br>
> }<br>
><br>
> +TEST_F(InstrProfTest, get_icall_data_merge1_saturation) {<br>
> + const uint64_t Max = std::numeric_limits<uint64_t>::max();<br>
> +<br>
> + InstrProfRecord Record1("caller", 0x1234, {1});<br>
> + InstrProfRecord Record2("caller", 0x1234, {1});<br>
> + InstrProfRecord Record3("callee1", 0x1235, {3, 4});<br>
> +<br>
> + Record1.reserveSites(IPVK_IndirectCallTarget, 1);<br>
> + InstrProfValueData VD1[] = {{(uint64_t) "callee1", 1}};<br>
> + Record1.addValueData(IPVK_IndirectCallTarget, 0, VD1, 1, nullptr);<br>
> +<br>
> + Record2.reserveSites(IPVK_IndirectCallTarget, 1);<br>
> + InstrProfValueData VD2[] = {{(uint64_t) "callee1", Max}};<br>
> + Record2.addValueData(IPVK_IndirectCallTarget, 0, VD2, 1, nullptr);<br>
> +<br>
> + Writer.addRecord(std::move(Record1));<br>
> + Writer.addRecord(std::move(Record2));<br>
> + Writer.addRecord(std::move(Record3));<br>
> +<br>
> + auto Profile = Writer.writeBuffer();<br>
> + readProfile(std::move(Profile));<br>
> +<br>
> + // Verify saturation of counts.<br>
> + ErrorOr<InstrProfRecord> R = Reader->getInstrProfRecord("caller", 0x1234);<br>
> + ASSERT_TRUE(NoError(R.getError()));<br>
> + ASSERT_EQ(1U, R.get().getNumValueSites(IPVK_IndirectCallTarget));<br>
> + std::unique_ptr<InstrProfValueData[]> VD =<br>
> + R.get().getValueForSite(IPVK_IndirectCallTarget, 0);<br>
> + ASSERT_EQ(StringRef("callee1"), StringRef((const char *)VD[0].Value, 7));<br>
> + ASSERT_EQ(Max, VD[0].Count);<br>
> +}<br>
> +<br>
> TEST_F(InstrProfTest, get_max_function_count) {<br>
> InstrProfRecord Record1("foo", 0x1234, {1ULL << 31, 2});<br>
> InstrProfRecord Record2("bar", 0, {1ULL << 63});<br>
><br>
> Modified: llvm/trunk/unittests/Support/MathExtrasTest.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/MathExtrasTest.cpp?rev=253497&r1=253496&r2=253497&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/MathExtrasTest.cpp?rev=253497&r1=253496&r2=253497&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/unittests/Support/MathExtrasTest.cpp (original)<br>
> +++ llvm/trunk/unittests/Support/MathExtrasTest.cpp Wed Nov 18 14:40:41 2015<br>
> @@ -190,4 +190,40 @@ TEST(MathExtras, RoundUpToAlignment) {<br>
> EXPECT_EQ(552u, RoundUpToAlignment(321, 255, 42));<br>
> }<br>
><br>
> +template<typename T><br>
> +void SaturatingAddTestHelper()<br>
> +{<br>
> + const T Max = std::numeric_limits<T>::max();<br>
> + EXPECT_EQ(T(3), SaturatingAdd(T(1), T(2)));<br>
> + EXPECT_EQ(Max, SaturatingAdd(Max, T(1)));<br>
> + EXPECT_EQ(Max, SaturatingAdd(T(1), Max));<br>
> + EXPECT_EQ(Max, SaturatingAdd(Max, Max));<br>
> +}<br>
> +<br>
> +TEST(MathExtras, SaturatingAdd) {<br>
> + SaturatingAddTestHelper<uint8_t>();<br>
> + SaturatingAddTestHelper<uint16_t>();<br>
> + SaturatingAddTestHelper<uint32_t>();<br>
> + SaturatingAddTestHelper<uint64_t>();<br>
> +}<br>
> +<br>
> +template<typename T><br>
> +void SaturatingMultiplyTestHelper()<br>
> +{<br>
> + const T Max = std::numeric_limits<T>::max();<br>
> + EXPECT_EQ(T(0), SaturatingMultiply(T(1), T(0)));<br>
> + EXPECT_EQ(T(0), SaturatingMultiply(T(0), T(1)));<br>
> + EXPECT_EQ(T(6), SaturatingMultiply(T(2), T(3)));<br>
> + EXPECT_EQ(Max, SaturatingMultiply(Max, T(2)));<br>
> + EXPECT_EQ(Max, SaturatingMultiply(T(2),Max));<br>
> + EXPECT_EQ(Max, SaturatingMultiply(Max, Max));<br>
> +}<br>
> +<br>
> +TEST(MathExtras, SaturatingMultiply) {<br>
> + SaturatingMultiplyTestHelper<uint8_t>();<br>
> + SaturatingMultiplyTestHelper<uint16_t>();<br>
> + SaturatingMultiplyTestHelper<uint32_t>();<br>
> + SaturatingMultiplyTestHelper<uint64_t>();<br>
> +}<br>
> +<br>
> }<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br>
<br>
<br>
</div></div><span><font color="#888888">--<br>
Bruno Cardoso Lopes<br>
<a href="http://www.brunocardoso.cc" rel="noreferrer" target="_blank">http://www.brunocardoso.cc</a><br>
</font></span><div><div>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>
</div></div><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>
</div></div><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>