Add option to use boost::multiprecision on platforms where int128_t i… #3430

chrchr-github · 2021-08-30T16:40:51Z

…s not available

pfultz2 · 2021-08-30T18:07:11Z

lib/CMakeLists.txt

+if (USE_BOOST_MULTIPREC)
+    find_package(Boost REQUIRED COMPONENTS system)
+    include_directories(${Boost_INCLUDE_DIRS})
+    add_compile_definitions(USE_BOOST_MULTIPREC)


This should use:

target_include_directories(lib_objs PUBLIC SYSTEM ${Boost_INCLUDE_DIRS}) target_compile_definitions(lib_objs PUBLIC USE_BOOST_MULTIPREC)

I guess this makes it specific to lib_objs?

That's add_definitions(-DUSE_BOOST_MULTIPREC) - target_compile_definitions() is not available in CMake 2.8 (let's tackle a CMake update later - if I ever get healthy again).

Also those should be placed in findDependencies.cmake and compilerDefinitions.cmake.

Also those should be PRIVATE so they don't spill into everything.

pfultz2 · 2021-08-30T18:25:11Z

Dont we need to update MathLib::bigint to make this useful for ValueFlow as well? As this changes only affect the bug-hunting.

pfultz2 · 2021-08-30T18:27:22Z

I guess the MathLib::bigint change could be made in another PR.

firewave · 2021-08-30T18:31:11Z

lib/exprengine.h

@@ -40,13 +40,18 @@ class Variable;
 #if defined(__GNUC__) && defined (__SIZEOF_INT128__)
 typedef __int128_t int128_t;
 #else
+#if defined(USE_BOOST_MULTIPREC)


This should be the first case. If we configure to use it we should use it across all platforms

firewave · 2021-08-30T18:33:44Z

lib/CMakeLists.txt

@@ -43,3 +43,9 @@ add_library(lib_objs OBJECT ${srcs_lib} ${hdrs})
 if (NOT CMAKE_DISABLE_PRECOMPILE_HEADERS)
    target_precompile_headers(lib_objs PRIVATE precompiled.h)
 endif()
+
+if (USE_BOOST_MULTIPREC)
+    find_package(Boost REQUIRED COMPONENTS system)


Why system? The component is just multiprecision.

danmar · 2021-08-30T18:35:38Z

I am skeptic about taking in a boost dependency even if it's optional. I assume this is a msvc problem primarily. Is there no cleaner way to get this in windows also?

firewave · 2021-08-30T18:37:00Z

lib/exprengine.cpp

@@ -997,7 +997,7 @@ ExprEngine::ConditionalValue::Vector ExprEngine::ArrayValue::read(ExprEngine::Va
                if (i->minValue >= 0 && i->minValue == i->maxValue) {
                    int c = 0;
                    if (i->minValue < stringLiteral->size())
-                        c = stringLiteral->string[i->minValue];
+                        c = stringLiteral->string[static_cast<size_t>(i->minValue)];


Since this is 128-bit now it will probably cause problems when it doesn't fit into size_t.

firewave · 2021-08-30T18:37:53Z

I am skeptic about taking in a boost dependency even if it's optional. I assume this is a msvc problem primarily. Is there no cleaner way to get this in windows also?

Maybe using clang-cl... and probably using Cygwin/MinGW.

danmar · 2021-08-30T18:41:15Z

Hi @firewave good to see you..

I think we should look into using clang-cl in the windows build. as far as I have seen it makes the cppcheck binary a lot faster. For information, there is a ticket here: https://trac.cppcheck.net/ticket/10441

And then for the bughunting I am not 100% sure it was a good idea to use 128 bit integer. The Z3 library does not support this at all so we get 64-bit integers anyway in all expressions. Maybe a redesign is better..

firewave · 2021-08-30T18:45:56Z

Hi @firewave good to see you..

Don't blink.

I think we should look into using clang-cl in the windows build. as far as I have seen it makes the cppcheck binary a lot faster.

Well - I think it's up to the actual user which toolchain is being used. And having something which is available on all platforms seems better. But I also feel uneasy about having a Boost dependency. I would prefer something much lighter.

And then for the bughunting I am not 100% sure it was a good idea to use 128 bit integer. The Z3 library does not support this at all so we get 64-bit integers anyway in all expressions. Maybe a redesign is better..

This was actually triggered by https://trac.cppcheck.net/ticket/9994 which has nothing to do with bug hunting.

danmar · 2021-08-30T18:54:17Z

This was actually triggered by https://trac.cppcheck.net/ticket/9994 which has nothing to do with bug hunting.

aha ok..

Do we have to make bigint 128 bit.. I have the feeling that some class like:

class bigint {
    uint64_t rawvalue;
    uint8_t bits;
    bool sign;
};

and then some convenient interface might work also..? and maybe that could handle calculations more properly also.

chrchr-github · 2021-08-30T18:57:47Z

I guess I won't waste time on CMake details until there is a consensus that this could be approved in principle...

danmar · 2021-08-30T19:01:52Z

thanks @chrchr-github that sounds reasonable.

pfultz2 · 2021-08-30T20:22:15Z

We could incorporate calccrypto/uint128_t into cppcheck. We would need to write some code to make this work for signed integers(although all the arithmetic operations should be the same only the conversions need updated) and we may need fill in some of the math functions.

However, using boost.multiprecision doesn't really need any extra code changes, so it seems simpler to integrate and then we can look into removing dependency in the future.

I have the feeling that some class like

I am not sure how easy that can be used in place of bigint. We would need to implement all of the operators for this, and bitwise operations may not work correctly since it no longer uses two's complement.

I think it would be best to use boost.multiprecision as an optional dependency now and then we can look at better options in the future.

Also, this is header only, so this does not require building boost nor will the final binaries have a dependency on boost.

firewave · 2021-08-30T21:03:39Z

Also, this is header only, so this does not require building boost nor will the final binaries have a dependency on boost.

Good point.

I guess I won't waste time on CMake details until there is a consensus that this could be approved in principle...

"waste" - it's "learning" ;)

firewave · 2021-09-04T11:12:42Z

BTW Please mark this a draft for now until the changes are finished/it has been decided about. Thanks.

chrchr-github added 2 commits August 30, 2021 18:40

Add option to use boost::multiprecision on platforms where int128_t i…

919c0e9

…s not available

tab -> spaces

6aa9c51

pfultz2 reviewed Aug 30, 2021

View reviewed changes

chrchr-github added 2 commits August 30, 2021 20:13

Set options only for lib_objs

731c9a6

Add options after target is defined

3425519

pfultz2 approved these changes Aug 30, 2021

View reviewed changes

firewave reviewed Aug 30, 2021

View reviewed changes

chrchr-github marked this pull request as draft September 4, 2021 12:07

chrchr-github closed this Nov 3, 2021

chrchr-github deleted the chr_int128 branch November 3, 2021 18:48

firewave mentioned this pull request Nov 21, 2024

added CMake option USE_BOOST_INT128 to use a Boost.Multiprecision 128-bit integer for MathLib:big{u}int #7028

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to use boost::multiprecision on platforms where int128_t i… #3430

Add option to use boost::multiprecision on platforms where int128_t i… #3430

chrchr-github commented Aug 30, 2021

pfultz2 Aug 30, 2021

chrchr-github Aug 30, 2021

firewave Aug 30, 2021

pfultz2 commented Aug 30, 2021

pfultz2 commented Aug 30, 2021

firewave Aug 30, 2021

firewave Aug 30, 2021

danmar commented Aug 30, 2021

firewave Aug 30, 2021

firewave commented Aug 30, 2021 •

edited

Loading

danmar commented Aug 30, 2021 •

edited

Loading

firewave commented Aug 30, 2021

danmar commented Aug 30, 2021 •

edited

Loading

chrchr-github commented Aug 30, 2021

danmar commented Aug 30, 2021

pfultz2 commented Aug 30, 2021

firewave commented Aug 30, 2021

firewave commented Sep 4, 2021

Add option to use boost::multiprecision on platforms where int128_t i… #3430

Add option to use boost::multiprecision on platforms where int128_t i… #3430

Conversation

chrchr-github commented Aug 30, 2021

pfultz2 Aug 30, 2021

Choose a reason for hiding this comment

chrchr-github Aug 30, 2021

Choose a reason for hiding this comment

firewave Aug 30, 2021

Choose a reason for hiding this comment

pfultz2 commented Aug 30, 2021

pfultz2 commented Aug 30, 2021

firewave Aug 30, 2021

Choose a reason for hiding this comment

firewave Aug 30, 2021

Choose a reason for hiding this comment

danmar commented Aug 30, 2021

firewave Aug 30, 2021

Choose a reason for hiding this comment

firewave commented Aug 30, 2021 • edited Loading

danmar commented Aug 30, 2021 • edited Loading

firewave commented Aug 30, 2021

danmar commented Aug 30, 2021 • edited Loading

chrchr-github commented Aug 30, 2021

danmar commented Aug 30, 2021

pfultz2 commented Aug 30, 2021

firewave commented Aug 30, 2021

firewave commented Sep 4, 2021

firewave commented Aug 30, 2021 •

edited

Loading

danmar commented Aug 30, 2021 •

edited

Loading

danmar commented Aug 30, 2021 •

edited

Loading