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

On C++11 and up support #3635

Closed
dimhotepus opened this issue Apr 24, 2020 · 23 comments
Closed

On C++11 and up support #3635

dimhotepus opened this issue Apr 24, 2020 · 23 comments

Comments

@dimhotepus
Copy link
Contributor

dimhotepus commented Apr 24, 2020

Does BOINC support C++11?

For example, i want to fix undefined behavior (which may lead to security issues) when printf-like functions get incompatible format specifier and actual parameter type: #3633 In order to do that, i either need to include <cinttypes> (C++11 thing) for portable format specifiers, or add ugly macroses to duplicate such functionality in BOINC headers.

If we support MinGW builds, things become even more complicated. MinGW uses old msvcrt runtime, and requires special define __USE_MINGW_ANSI_STDIO=1 to mimic C11 behavior (%zu format specifier for size_t). This also can lead to changes in how other format specifiers work on MinGW builds.

So possible solution is to

  1. Define all needed format specifier macroses manually (including special cases for MinGW) in BOINC headers. Hard to do correctly, but possible (Visual Studio added support of different format specifiers from different versions, MinGW with its own formatters, 32 vs 64 bit types, etc).

or

Allow C++11 in gcc builds (as it's done for Windows builds on vs2019) and

  1. Define special portable format specifiers based on <cinttypes> for MinGW.
    or
  2. Define __USE_MINGW_ANSI_STDIO=1 for MinGW builds and use standard format specifiers from <cinttypes>.
    or
  3. Drop support of MinGW and use VsCode or Community versions of Visual Studio to compile BOINC on Windows. So cross-compiling Windows builds on Linux will not work.

What do you think?

@AenBleidd
Copy link
Member

Currently c++11 is not supported. The reason for this is that current Windows release builds are still done on MS VS2010 that is not support c++11.
Also, do we really need ? Is there another way to print values without these weird format specifiers like PRIuMAX ?

@dimhotepus
Copy link
Contributor Author

dimhotepus commented Apr 24, 2020

It is hard to do in a cross-(platform/compiler) way. Especially if we support old Visual Studio versions, like vs2010.

Portable format specifiers were added in inttypes.h/cinttypes as a part of C++11. So we stuck with preprocessor definitions similar to PRIuMAX. Something like this (add MinGW and MSC_VER to the mix):

#ifdef _WIN32
#define PR_SIZET 
#elif defined(_WIN64)
#define PR_SIZET "I"
#elif defined(__linux__) 
#if defined(__x86_64__)
#define PR_SIZET  "z"
#else
#define PR_SIZET
#endif
#else
#error Please, define PR_SIZET for your platform
#endif



size_t u;
printf("%" PR_SIZET "x", u);

@davidpanderson
Copy link
Contributor

Is there a bug here, or is it just about compiler warnings?

@dimhotepus
Copy link
Contributor Author

dimhotepus commented Apr 25, 2020

In general, if the syntax of a conversion specification is invalid, behavior is undefined, and can cause program termination.

For example,
at https://github.com/BOINC/boinc/pull/3633/files#diff-af2978c2e9a6dcbd81e11d25b397ecadR1284 we interpret actual data (size_t, unsigned, 8 bytes long on x86-64) as int (4 bytes, signed), which can lead to dumping incorrect process statistics values when they are > INT_MAX.

at https://github.com/BOINC/boinc/pull/3633/files#diff-af2978c2e9a6dcbd81e11d25b397ecadR1434 and https://github.com/BOINC/boinc/pull/3633/files#diff-af2978c2e9a6dcbd81e11d25b397ecadR1444 we output incorrect addresses of exception sources on x86-64 when they > UINT_MAX.

at https://github.com/BOINC/boinc/pull/3633/files#diff-2100491c40a00d1c0a6c54ed6a816619R732 we dump incorrect cpu registers / stack frame values on x86-64 when they > UINT_MAX.

@sirzooro
Copy link
Contributor

sirzooro commented Apr 30, 2020

I recall that I used inttypes.h around year 2007 on Linux and Solaris, before I even heard about C++11. BOINC could check in ./configure if this header and macros are present and use them. If not, it could provide its own ones.

@dimhotepus
Copy link
Contributor Author

dimhotepus commented May 2, 2020

Problem is gcc rejects to build with inttypes.h if -std=c++11 not passed as compiler option.

@dimhotepus
Copy link
Contributor Author

Decided to define portable size_t printf format specifier across different compilers.

@cminnoy
Copy link

cminnoy commented Feb 10, 2021

Wondering what the reason is to still support Visual Studio 2010.

@cnergyone
Copy link

Ain't there a way to upgrade to a newer Visual Studio?

Secondly, I noticed that clearly on Ubuntu C++11 features compile fine for BOINC. So if no C++11 features are allowed, than the compiler options should specify the standard to enforce compliance.

@AenBleidd
Copy link
Member

@cnergyone,

Ain't there a way to upgrade to a newer Visual Studio?

It's already done. But while there is no new release for Windows with VS2019 - we don't drop VS2010 support.

So if no C++11 features are allowed, than the compiler options should specify the standard to enforce compliance.

Because next release we want to make with VS2019, so no need to add this restriction now.

So this ticket will leave for a while and soon will be closed without any fix.

@cnergyone
Copy link

I'm implementing ticket 4180. You're saying that I can now use C++11 statements?

What will be the new standard after the next release? C++11, C++14 or C++17?
I vote for C++17

@AenBleidd
Copy link
Member

I'm implementing ticket 4180. You're saying that I can now use C++11 statements?

After BOINC will be released for Windows and I close this ticket -
yes.

Regarding next standard - I don't know yet.

@dimhotepus
Copy link
Contributor Author

I'm implementing ticket 4180. You're saying that I can now use C++11 statements?

After BOINC will be released for Windows and I close this ticket -
yes.

Regarding next standard - I don't know yet.

Is it correct that after #4207 be merged I can use <cinttypes> header from C++11 standard library to deal with #3633 long running PR?

@AenBleidd
Copy link
Member

@dimhotepus, I'd ask you to wait until next Windows release. Then coding style document should be updated with new requirements. #4207 was merged because it touched only linux code. For Windows we still want to have a fallback solution in case smth goes wrong. I'll inform you when there will be a green light for your PR ;)

@cnergyone
Copy link

Any progress on the C++11 adoption? I noticed my pull request still fails on Android and OSX manager builds because of lack of C++11 support (auto keyword and lambda).

@AenBleidd
Copy link
Member

@cnergyone, no updates right now. I'll notify you about it

@CharlieFenton
Copy link
Contributor

@cnergyone My understanding is that current versions of Xcode on MacOS enable C++11 support by default. But I'm not certain of that. You can test this by modifying mac_build/buildMacBOINC-CI.sh to pass the argument -c++11 to each call of source mac_build/buildMacBOINC.sh.

Let me know if making those changes fixes the errors; that would indicate that apparently C++11 support is not enabled by default. If so, I'll update the Xcode project and buildMacBOINC.sh script accordingly.

@CharlieFenton
Copy link
Contributor

I noticed my pull request still fails on Android and OSX manager builds because of lack of C++11 support (auto keyword and lambda).

@cnergyone Which pull request is failing on MacOS Manager due to those keywords? The only PR I'm finding from you is #4181 and the error is not due to either of those keywords.

@CharlieFenton
Copy link
Contributor

@cnergyone Actually, GitHub is a bit confusing when I search for Pull Requests authored by you, it says #4181 but it seems to be #4207. Either way, the error for MacOS Manager is not due to either the auto keyword or lambda.

@CharlieFenton
Copy link
Contributor

@cnergyone Sorry, as I've dug into this a bit more I realize I'm not familiar with more advanced concepts in C++11 such as lambda expressions. But I have confirmed that by specifying C++11 support, your code does compile on MacOS without generating any errors. However, specifying C++11 support on MacOS does generate warnings and errors in other source files.

I will try to submit a PR in the next week or two to enable C++11 support on macOS.

@CharlieFenton
Copy link
Contributor

@cnergyone @AenBleidd I have completed the changes for C++11 support on MacOS in my PR #4304.

@AenBleidd
Copy link
Member

Please be aware that we're not ready now to accept any c++11 specific features. When next release for Windows will be released with no issues using VS2019, then c++11 support will be officially announced.
Please be patient and stay tuned.

@AenBleidd
Copy link
Member

Currently BOINC has limited support of c++11 and later standards.
However, not all the components of BOINC could be written with it (e.g. libboinc should not use c++11 to keep backward compatibility with the older platforms and/or compilers).
At this moment we have quite a stable CI that covers the most important platforms and configurations, and it could be used to understand, if the code is compatible or not.

But even if I say that BOIN Chas limited support of c++11 and later, that doesn't mean that we will rewrite any part of the existing code to the newer c++ standard.
Usage of the c++11 and later is acceptable while working on some bug/feature and only on those parts of the code that are directly affected by that bug/feature.

The current codebase is stable enough, so there is no reason to put any efforts to rewrite any part of it with the newer c++ standard.

@AenBleidd AenBleidd closed this as not planned Won't fix, can't repro, duplicate, stale Aug 29, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in Planning Aug 29, 2024
@github-project-automation github-project-automation bot moved this from Prioritized to Done in Client/Manager Aug 29, 2024
@AenBleidd AenBleidd removed the T: Task label Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Archived in project
Development

No branches or pull requests

7 participants