Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

protect CGAL against macro free #8523

Merged
merged 7 commits into from
Oct 18, 2024

Conversation

lrineau
Copy link
Member

@lrineau lrineau commented Oct 7, 2024

Summary of Changes

From vcpkg CI, there is this error:

123456789101112131415161718192021222324252627C:\PROGRA~1\MICROS~1\2022\ENTERP~1\VC\Tools\MSVC\1441~1.341\bin\Hostx64\x64\cl.exe   /TP -DBOOST_ATOMIC_DYN_LINK -DBOOST_ATOMIC_NO_LIB -DBOOST_CHRONO_DYN_LINK -DBOOST_CHRONO_NO_LIB -DBOOST_CONTAINER_DYN_LINK -DBOOST_CONTAINER_NO_LIB -DBOOST_DATE_TIME_DYN_LINK -DBOOST_DATE_TIME_NO_LIB -DBOOST_IOSTREAMS_DYN_LINK -DBOOST_IOSTREAMS_NO_LIB -DBOOST_PROGRAM_OPTIONS_DYN_LINK -DBOOST_PROGRAM_OPTIONS_NO_LIB -DBOOST_RANDOM_DYN_LINK -DBOOST_RANDOM_NO_LIB -DBOOST_SERIALIZATION_DYN_LINK -DBOOST_SERIALIZATION_NO_LIB -DBOOST_THREAD_DYN_LINK -DBOOST_THREAD_NO_LIB -DBOOST_THREAD_USE_DLL -DCGAL_USE_GMPXX=1 -DGFLAGS_IS_A_DLL=1 -DGLOG_NO_ABBREVIATED_SEVERITIES -DGLOG_USE_GFLAGS -DGLOG_USE_GLOG_EXPORT -DH5_BUILT_AS_DYNAMIC_LIB -D_LIB -D_USE_BOOST -D_USE_EIGEN -D_USE_FAST_CBRT -D_USE_FAST_FLOAT2INT -D_USE_NONFREE -D_USE_OPENGL -D_USE_SSE -ID:\b\openmvs\x64-windows-dbg\libs\MVS\MVS_autogen\include -ID:\b\openmvs\src\v2.1.0-1e694de437.clean -ID:\b\openmvs\x64-windows-dbg -external:ID:\installed\x64-windows\include -external:ID:\installed\x64-windows\include\eigen3 -external:W0 /nologo /DWIN32 /D_WINDOWS /utf-8 /GR /EHsc /MP   /D _CRT_SECURE_NO_DEPRECATE /D _CRT_NONSTDC_NO_DEPRECATE /D _SCL_SECURE_NO_WARNINGS /Gy /bigobj /Oi /Zm170 /Zc:__cplusplus  /wd4231 /wd4251 /wd4308 /wd4396 /wd4503 /wd4661 /wd4996 /D_DEBUG /MDd /Z7 /Ob0 /Od /RTC1   -std:c++17 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS /fp:strict /fp:except- /bigobj /Zc:__cplusplus /YuD:/b/openmvs/x64-windows-dbg/libs/MVS/CMakeFiles/MVS.dir/cmake_pch.hxx /FpD:/b/openmvs/x64-windows-dbg/libs/MVS/CMakeFiles/MVS.dir/./cmake_pch.cxx.pch /FID:/b/openmvs/x64-windows-dbg/libs/MVS/CMakeFiles/MVS.dir/cmake_pch.hxx /showIncludes /Folibs\MVS\CMakeFiles\MVS.dir\SceneReconstruct.cpp.obj /Fdlibs\MVS\CMakeFiles\MVS.dir\MVS.pdb /FS -c D:\b\openmvs\src\v2.1.0-1e694de437.clean\libs\MVS\SceneReconstruct.cpp
D:\installed\x64-windows\include\CGAL/CORE/MemoryPool.h(76): error C2059: syntax error: 'constant'
D:\installed\x64-windows\include\CGAL/CORE/MemoryPool.h(76): note: the template instantiation context (the oldest one first) is
D:\installed\x64-windows\include\CGAL/CORE/MemoryPool.h(39): note: while compiling class template 'CORE::MemoryPool'
D:\installed\x64-windows\include\CGAL/CORE/MemoryPool.h(119): error C2988: unrecognizable template declaration/definition
D:\installed\x64-windows\include\CGAL/CORE/MemoryPool.h(119): error C2059: syntax error: 'constant'
D:\installed\x64-windows\include\CGAL/CORE/Expr_impl.h(1217): error C2660: 'CORE::MemoryPool<CORE::ConstDoubleRep,1024>::_free_dbg': function does not take 2 arguments
D:\installed\x64-windows\include\CGAL/CORE/MemoryPool.h(76): note: see declaration of 'CORE::MemoryPool<CORE::ConstDoubleRep,1024>::_free_dbg'
D:\installed\x64-windows\include\CGAL/CORE/Expr_impl.h(1217): note: while trying to match the argument list '(void *, int)'

There is probably a macro named free somewhere.


Update: If the macros _DEBUG and _CRTDBG_MAP_ALLOC are defined, then the Windows runtime headers redefine free and malloc.

This PR also adds a test for _CRTDBG_MAP_ALLOC in the CGAL test suite.

Release Management

@lrineau lrineau added Bug Pkg::CGAL_Core upstream bug Bugs in third party software used by CGAL labels Oct 7, 2024
@lrineau lrineau added this to the 6.0.1 milestone Oct 7, 2024
lrineau added a commit to lrineau/vcpkg that referenced this pull request Oct 7, 2024
patch from upstream PR CGAL/cgal#8523
From vcpkg CI, there is this error:

```
123456789101112131415161718192021222324252627C:\PROGRA~1\MICROS~1\2022\ENTERP~1\VC\Tools\MSVC\1441~1.341\bin\Hostx64\x64\cl.exe   /TP -DBOOST_ATOMIC_DYN_LINK -DBOOST_ATOMIC_NO_LIB -DBOOST_CHRONO_DYN_LINK -DBOOST_CHRONO_NO_LIB -DBOOST_CONTAINER_DYN_LINK -DBOOST_CONTAINER_NO_LIB -DBOOST_DATE_TIME_DYN_LINK -DBOOST_DATE_TIME_NO_LIB -DBOOST_IOSTREAMS_DYN_LINK -DBOOST_IOSTREAMS_NO_LIB -DBOOST_PROGRAM_OPTIONS_DYN_LINK -DBOOST_PROGRAM_OPTIONS_NO_LIB -DBOOST_RANDOM_DYN_LINK -DBOOST_RANDOM_NO_LIB -DBOOST_SERIALIZATION_DYN_LINK -DBOOST_SERIALIZATION_NO_LIB -DBOOST_THREAD_DYN_LINK -DBOOST_THREAD_NO_LIB -DBOOST_THREAD_USE_DLL -DCGAL_USE_GMPXX=1 -DGFLAGS_IS_A_DLL=1 -DGLOG_NO_ABBREVIATED_SEVERITIES -DGLOG_USE_GFLAGS -DGLOG_USE_GLOG_EXPORT -DH5_BUILT_AS_DYNAMIC_LIB -D_LIB -D_USE_BOOST -D_USE_EIGEN -D_USE_FAST_CBRT -D_USE_FAST_FLOAT2INT -D_USE_NONFREE -D_USE_OPENGL -D_USE_SSE -ID:\b\openmvs\x64-windows-dbg\libs\MVS\MVS_autogen\include -ID:\b\openmvs\src\v2.1.0-1e694de437.clean -ID:\b\openmvs\x64-windows-dbg -external:ID:\installed\x64-windows\include -external:ID:\installed\x64-windows\include\eigen3 -external:W0 /nologo /DWIN32 /D_WINDOWS /utf-8 /GR /EHsc /MP   /D _CRT_SECURE_NO_DEPRECATE /D _CRT_NONSTDC_NO_DEPRECATE /D _SCL_SECURE_NO_WARNINGS /Gy /bigobj /Oi /Zm170 /Zc:__cplusplus  /wd4231 /wd4251 /wd4308 /wd4396 /wd4503 /wd4661 /wd4996 /D_DEBUG /MDd /Z7 /Ob0 /Od /RTC1   -std:c++17 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS /fp:strict /fp:except- /bigobj /Zc:__cplusplus /YuD:/b/openmvs/x64-windows-dbg/libs/MVS/CMakeFiles/MVS.dir/cmake_pch.hxx /FpD:/b/openmvs/x64-windows-dbg/libs/MVS/CMakeFiles/MVS.dir/./cmake_pch.cxx.pch /FID:/b/openmvs/x64-windows-dbg/libs/MVS/CMakeFiles/MVS.dir/cmake_pch.hxx /showIncludes /Folibs\MVS\CMakeFiles\MVS.dir\SceneReconstruct.cpp.obj /Fdlibs\MVS\CMakeFiles\MVS.dir\MVS.pdb /FS -c D:\b\openmvs\src\v2.1.0-1e694de437.clean\libs\MVS\SceneReconstruct.cpp
D:\installed\x64-windows\include\CGAL/CORE/MemoryPool.h(76): error C2059: syntax error: 'constant'
D:\installed\x64-windows\include\CGAL/CORE/MemoryPool.h(76): note: the template instantiation context (the oldest one first) is
D:\installed\x64-windows\include\CGAL/CORE/MemoryPool.h(39): note: while compiling class template 'CORE::MemoryPool'
D:\installed\x64-windows\include\CGAL/CORE/MemoryPool.h(119): error C2988: unrecognizable template declaration/definition
D:\installed\x64-windows\include\CGAL/CORE/MemoryPool.h(119): error C2059: syntax error: 'constant'
D:\installed\x64-windows\include\CGAL/CORE/Expr_impl.h(1217): error C2660: 'CORE::MemoryPool<CORE::ConstDoubleRep,1024>::_free_dbg': function does not take 2 arguments
D:\installed\x64-windows\include\CGAL/CORE/MemoryPool.h(76): note: see declaration of 'CORE::MemoryPool<CORE::ConstDoubleRep,1024>::_free_dbg'
D:\installed\x64-windows\include\CGAL/CORE/Expr_impl.h(1217): note: while trying to match the argument list '(void *, int)'
```

There is probably a macro named `free` somewhere.
@lrineau lrineau force-pushed the CGAL_CGAL-protect_against_macro_free-GF branch from 590724e to cab1117 Compare October 7, 2024 13:05
lrineau added a commit to lrineau/vcpkg that referenced this pull request Oct 7, 2024
patch from upstream PR CGAL/cgal#8523
@lrineau lrineau changed the title CGAL_Core: protect against macro free protect CGAL against macro free Oct 8, 2024
@lrineau lrineau added Pkg::BGL Pkg::PMP Pkg::Classification and removed upstream bug Bugs in third party software used by CGAL labels Oct 8, 2024
@afabri
Copy link
Member

afabri commented Oct 9, 2024

This also concerns TBB, in I-c-343 so not in our responsability. No idea if that can be protected with an `#undef`` and if we should do that.

@afabri
Copy link
Member

afabri commented Oct 9, 2024

We also have the error I-c-343 (see code), when initializing a data member called free.

@afabri
Copy link
Member

afabri commented Oct 9, 2024

Concering TBB I suggest to upgrade it on rothko. It is TBB 2019. Already with 2022 on my machine I cannot reproduce. @janetournois @soesau can you do the upgrade please.

@sloriot
Copy link
Member

sloriot commented Oct 14, 2024

Successfully tested in CGAL-6.0.1-Ic-345

@sloriot
Copy link
Member

sloriot commented Oct 14, 2024

@lrineau still marked as draft. If you are done with it, please mark it as "Ready for review" so that it can be merged.

@lrineau lrineau marked this pull request as ready for review October 14, 2024 07:58
@lrineau
Copy link
Member Author

lrineau commented Oct 14, 2024

@lrineau still marked as draft. If you are done with it, please mark it as "Ready for review" so that it can be merged.

You are right. I have marked it as ready for review.

@sloriot @afabri, I am still unsure about the patch of <CGAL/config.h>:

#if defined(_MSC_VER) && defined(_DEBUG)
// Include support for memory leak detection
// This is only available in debug mode and when _CRTDBG_MAP_ALLOC is defined.
// It will include <crtdbg.h> which will redefine `malloc` and `free`.
// # define _CRTDBG_MAP_ALLOC 1
#endif

I really would have liked that we fix the issue with TBB, and keep testing with #define _CRTDBG_MAP_ALLOC 1 every day. That is the only way to make sure the issue will not resurface.

@github-actions github-actions bot removed the Tested label Oct 14, 2024
Copy link

This pull-request was previously marked with the label Tested, but has been modified with new commits. That label has been removed.

@afabri
Copy link
Member

afabri commented Oct 15, 2024

I guess the warning in [I-c-348](
10>C:\dev\SCIP\lib\cmake\scip......\include\scip/def.h(109): warning C4005: 'getcwd': macro redefinition [C:\CGAL_ROOT\CGAL-6.0.1-Ic-348\cmake\platforms\MSVC2017-Debug-64bits\test\Polygonal_surface_reconstruction_Examples\polyfit_example_user_provided_planes.vcxproj]
C:\Program Files (x86)\Windows Kits\10\Include\10.0.22621.0\ucrt\crtdbg.h(327): note: see previous definition of 'getcwd') is triggered by this PR. @soesau can you please check if the two definitions are identical.

@afabri
Copy link
Member

afabri commented Oct 17, 2024

I reported an issue in the scip project on github.

@afabri
Copy link
Member

afabri commented Oct 18, 2024

The scip project has fixed the warning. As we installed scip with vcpkg as @soesau told me, we will have the warning for a while in the testsuite.

@sloriot
Copy link
Member

sloriot commented Oct 18, 2024

Successfully tested in CGAL-6.0.1-Ic-351

@sloriot sloriot merged commit e7b8a4e into CGAL:master Oct 18, 2024
9 checks passed
@lrineau lrineau deleted the CGAL_CGAL-protect_against_macro_free-GF branch October 21, 2024 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants