Skip to content

Fix wrong config defines in boost/windows.cfg #6492

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.

Sign up for GitHub

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

Merged
firewave merged 4 commits into from
Jun 20, 2024
Merged

Fix wrong config defines in boost/windows.cfg #6492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c866178
Fix defines for boost test macros
ChristophStrehle Jun 7, 2024
ea62369
Fix wrong defined in windows.cfg
ChristophStrehle Jun 7, 2024
7e50271
Add tests for boost config
ChristophStrehle Jun 7, 2024
abd8b11
Add tests for windows config
ChristophStrehle Jun 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions cfg/boost.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,23 +35,23 @@
<define name="BOOST_TEST(condition, ...)" value="static_cast&lt;void&gt;(static_cast&lt;bool&gt;(condition))" />
<define name="BOOST_TEST_REQUIRE(condition, ...)" value="static_cast&lt;void&gt;(static_cast&lt;bool&gt;(condition))" />
<define name="BOOST_WARN(condition)" value="static_cast&lt;void&gt;(static_cast&lt;bool&gt;(condition))" />
<define name="BOOST_WARN_MESSAGE(condition, msg)" value="static_cast&lt;void&gt;(static_cast&lt;bool&gt;(condition)); std::string(msg)" />
<define name="BOOST_WARN_MESSAGE(condition, msg)" value="static_cast&lt;void&gt;(static_cast&lt;bool&gt;(condition)); static_cast&lt;void&gt;(msg)" />
<define name="BOOST_WARN_EQUAL(a, b)" value="static_cast&lt;void&gt;((a) == (b))" />
<define name="BOOST_WARN_NE(a, b)" value="static_cast&lt;void&gt;((a) != (b))" />
<define name="BOOST_WARN_GT(a, b)" value="static_cast&lt;void&gt;((a) &gt; (b))" />
<define name="BOOST_WARN_GE(a, b)" value="static_cast&lt;void&gt;((a) &gt;= (b))" />
<define name="BOOST_WARN_LT(a, b)" value="static_cast&lt;void&gt;((a) &lt; (b))" />
<define name="BOOST_WARN_LE(a, b)" value="static_cast&lt;void&gt;((a) &lt;= (b))" />
<define name="BOOST_CHECK(condition)" value="static_cast&lt;void&gt;(static_cast&lt;bool&gt;(condition))" />
<define name="BOOST_CHECK_MESSAGE(condition, msg)" value="static_cast&lt;void&gt;(static_cast&lt;bool&gt;(condition)); std::string(msg)" />
<define name="BOOST_CHECK_MESSAGE(condition, msg)" value="static_cast&lt;void&gt;(static_cast&lt;bool&gt;(condition)); static_cast&lt;void&gt;(msg)" />
<define name="BOOST_CHECK_EQUAL(a, b)" value="static_cast&lt;void&gt;((a) == (b))" />
<define name="BOOST_CHECK_NE(a, b)" value="static_cast&lt;void&gt;((a) != (b))" />
<define name="BOOST_CHECK_GT(a, b)" value="static_cast&lt;void&gt;((a) &gt; (b))" />
<define name="BOOST_CHECK_GE(a, b)" value="static_cast&lt;void&gt;((a) &gt;= (b))" />
<define name="BOOST_CHECK_LT(a, b)" value="static_cast&lt;void&gt;((a) &lt; (b))" />
<define name="BOOST_CHECK_LE(a, b)" value="static_cast&lt;void&gt;((a) &lt;= (b))" />
<define name="BOOST_REQUIRE(condition)" value="static_cast&lt;void&gt;(static_cast&lt;bool&gt;(condition))" />
<define name="BOOST_REQUIRE_MESSAGE(condition, msg)" value="static_cast&lt;void&gt;(static_cast&lt;bool&gt;(condition)); std::string(msg)" />
<define name="BOOST_REQUIRE_MESSAGE(condition, msg)" value="static_cast&lt;void&gt;(static_cast&lt;bool&gt;(condition)); static_cast&lt;void&gt;(msg)" />
<define name="BOOST_REQUIRE_EQUAL(a, b)" value="static_cast&lt;void&gt;(static_cast&lt;bool&gt;((a) == (b)))" />
<define name="BOOST_REQUIRE_NE(a, b)" value="static_cast&lt;void&gt;((a) != (b))" />
<define name="BOOST_REQUIRE_GT(a, b)" value="static_cast&lt;void&gt;((a) &gt; (b))" />
Expand Down Expand Up @@ -84,6 +84,7 @@
<define name="BOOST_MATH_INT_VALUE_SUFFIX" value=""/>
<!-- Tell cppcheck to interpret BOOST_AUTO_TEST_CASE as a function definition -->
<define name="BOOST_AUTO_TEST_CASE(...)" value="void BOOST_AUTO_TEST_CASE_run(__VA_ARGS__)"/>
<define name="BOOST_AUTO_TEST_CASE_TEMPLATE(test_name, type_name, TL)" value="template&lt;typename type_name&gt;void test_name()"/>
<define name="BOOST_FIXTURE_TEST_CASE(name, fixture, ...)" value="struct name : fixture { void test_method(); }; void name::test_method()" />
<define name="BOOST_FIXTURE_TEST_CASE_TEMPLATE(test_name, type_name, TL, F)" value="template&lt;typename type_name&gt; struct test_name : public F { void test_method(); }; template&lt;typename type_name&gt; void test_name&lt;type_name&gt;::test_method()" />
<define name="BOOST_DATA_TEST_CASE(...)" value="void BOOST_DATA_TEST_CASE_run(__VA_ARGS__)"/>
Expand Down
8 changes: 6 additions & 2 deletions cfg/windows.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7119,8 +7119,8 @@ HFONT CreateFont(
<define name="afx_msg" value=""/>
<define name="AFX_EXT_CLASS" value=""/>
<define name="DEBUG_NEW" value="new"/>
<define name="INVALID_HANDLE_VALUE" value="0"/>
<define name="INVALID_SOCKET" value="0"/>
<define name="INVALID_HANDLE_VALUE" value="-1"/>
<define name="INVALID_SOCKET" value="~0"/>
Copy link
Copy Markdown
Collaborator

@danmar danmar Jun 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should INVALID_SOCKET also be -1 ? ~0 is -1 if 2-complement is used.

Copy link
Copy Markdown
Contributor Author

@ChristophStrehle ChristophStrehle Jun 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 would do the same thing as ~0. I decided to use ~0 as INVALID_SOCKET is defined like this in winsock.h, probably because SOCKET is unsigned. Should I rather use -1?

Copy link
Copy Markdown
Collaborator

@firewave firewave Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there actual documentation of the value of INVALID_SOCKET? IIRC depending on the license we cannot simply copy contents from existing headers as that would be a violation - MinGW (or MSYS?) got into hot water about this a long time ago - I cannot find anything about it though.

https://learn.microsoft.com/en-us/windows/win32/winsock/socket-data-type-2 seems to imply it is -1 or negative.

Copy link
Copy Markdown
Collaborator

@firewave firewave Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might have actually been Cygwin and might be related to this: https://www.cygwin.com/faq.html#faq.programming.win32-headers.

Copy link
Copy Markdown
Collaborator

@firewave firewave Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use the headers of MinGW as source but that licensing seems to be quite convoluted with a mixture of various ones depending on the file.

Copy link
Copy Markdown
Collaborator

@chrchr-github chrchr-github Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even Microsoft cannot copyright the value of a constant, and we are not copying/redistributing their headers or implementing their API anyway.
What's more concerning is that we define SOCKET as just int, whereas it should be __int64 or equivalent on 64bit platforms. But that's probably out of scope for this PR.

Copy link
Copy Markdown
Contributor Author

@ChristophStrehle ChristophStrehle Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I can't find any documentation on the value of INVALID_SOCKET and I can't find a mirror of the windows headers that I could refer to.
I came up with the ~0 because I looked this up in the WinSock2.h on my Windows machine. I did not copy paste anything as this would introduce yet another warning because of the C-style cast used there to cast the ~0 into a SOCKET. The SOCKET itself is defined as a UINT_PTR and this is defined in BaseTsd.h as unsigned __int64 for 64 bit or as unsigned int for 32 bit.

Here:
https://learn.microsoft.com/en-us/windows/win32/winsock/handling-winsock-errors
they mention a value of 0xffff which is probably not correct on a 64 bit machine.

Here I found a winsock.h in the Microsoft repo on GitHub:
https://github.com/microsoft/service-fabric/blob/bc5c2bd546e98812509330273ae1bc6502276f87/src/prod/src/pal/src/winsock2.h#L56
there the INVALID_SOCKET is the same as on my machine but there SOCKET is only an int.

Copy link
Copy Markdown
Collaborator

@firewave firewave Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure how to handle this. I personally only added stuff which was explicitly documented but I cannot speak for other submissions. I just jumped on this because you explicitly mentioned the original header as a source.

Maybe this is just an issue of re-licensing so maybe the configuration files simply need to be put into the public domain instead of GPL. Or it is even a complete non-issue as we are not using it in code.

CC @danmar

Copy link
Copy Markdown
Contributor Author

@ChristophStrehle ChristophStrehle Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@firewave thanks for the hint, I have to admit that I hadn't paid enough attention to the license but I don't think we have a licensing issue here. I did not copy any code from any Microsoft header into the code that is redistributed here. I only used the original headers form the sdk as a source of information for the values and for my commit message.

What I used can be found under the MIT license or the BSD license in the Microsoft GitHub repos as well:
https://github.com/microsoft/service-fabric/blob/bc5c2bd546e98812509330273ae1bc6502276f87/src/prod/src/pal/src/winsock2.h#L56
https://github.com/microsoft/mu_plus/blob/2d6746b8fe04a9be3998b3e1a682a4cbffcd7345/XmlSupportPkg/Library/XmlTreeLib/fasterxml/fasterxml.h#L949

So I'm pretty sure that this was fine. However, I have now removed the explicit mention from the commit message and form the comments above.

Copy link
Copy Markdown
Collaborator

@firewave firewave Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Thanks for looking into this.

As this was quite a while ago (probably 15+ years - maybe even longer) it is likely the license has changed. Also it is no longer the Microsoft of old (at least in relation to Linux/open-source).

<define name="WINAPI" value="__stdcall"/>
<define name="STDMETHODCALLTYPE" value="__stdcall"/>
<define name="STDMETHODIMP" value="HRESULT STDMETHODCALLTYPE"/>
Expand Down Expand Up @@ -14178,6 +14178,10 @@ HFONT CreateFont(
<define name="SIID_CLUSTEREDDRIVE" value="140"/>
<define name="SIID_MAX_ICONS" value="181"/>
<!-- constants from winnt.h -->
<define name="GENERIC_READ" value="0x80000000L" />
<define name="GENERIC_WRITE" value="0x40000000L" />
<define name="GENERIC_EXECUTE" value="0x20000000L" />
<define name="GENERIC_ALL" value="0x10000000L" />
<define name="LANG_AFRIKAANS" value="0x36"/>
<define name="LANG_ALBANIAN" value="0x1c"/>
<define name="LANG_ALSATIAN" value="0x84"/>
Expand Down
24 changes: 23 additions & 1 deletion test/cfg/boost.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include <boost/smart_ptr/scoped_array.hpp>
#include <boost/thread/mutex.hpp>
#include <boost/thread/lock_guard.hpp>
#include <boost/test/unit_test.hpp>

BOOST_FORCEINLINE void boost_forceinline_test()
{}
Expand Down Expand Up @@ -104,4 +105,25 @@ void lock_guard_finiteLifetime(boost::mutex& m)
{
// cppcheck-suppress unusedScopedObject
boost::lock_guard<boost::mutex>{ m };
}
}

BOOST_AUTO_TEST_SUITE(my_auto_test_suite)

BOOST_AUTO_TEST_CASE(test_message_macros)
{
bool my_bool = false;
BOOST_WARN_MESSAGE(my_bool, "warn");
BOOST_CHECK_MESSAGE(my_bool, "check");
BOOST_REQUIRE_MESSAGE(my_bool, "require");

BOOST_WARN_MESSAGE(my_bool, "my_bool was: " << my_bool);
}

using test_types_w_tuples = std::tuple<int, long, unsigned char>;
BOOST_AUTO_TEST_CASE_TEMPLATE(my_tuple_test, T, test_types_w_tuples)
Comment thread
chrchr-github marked this conversation as resolved.
{
// cppcheck-suppress valueFlowBailoutIncompleteVar
BOOST_TEST(sizeof(T) == 4U);
Comment thread
danmar marked this conversation as resolved.
}

BOOST_AUTO_TEST_SUITE_END()
20 changes: 20 additions & 0 deletions test/cfg/windows.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,26 @@
#include <atlstr.h>
#include <string>

void invalidHandle_CreateFile(LPCWSTR lpFileName)
{
HANDLE file = CreateFile(lpFileName, GENERIC_READ, 0, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr);
Comment thread
danmar marked this conversation as resolved.

// INVALID_HANDLE_VALUE is not the same as 0
if (file != INVALID_HANDLE_VALUE && file) {}

// cppcheck-suppress resourceLeak
}

void invalid_socket()
{
SOCKET sock = socket(AF_INET, SOCK_RAW, IPPROTO_TCP);

// INVALID_SOCKET is not the same as 0
if (sock != INVALID_SOCKET && sock) {}

// cppcheck-suppress resourceLeak
}

void resourceLeak_OpenThread(const DWORD dwDesiredAccess, const BOOL bInheritHandle, const DWORD dwThreadId)
{
HANDLE proc = OpenThread(dwDesiredAccess, bInheritHandle, dwThreadId);
Expand Down