Skip to content

ci: document that -Wreturn-type has been fixed upstream (mingw-w64) #28092

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

Jump to bottom
Merged
merged 1 commit into from
Jul 27, 2023
Merged

ci: document that -Wreturn-type has been fixed upstream (mingw-w64) #28092

merged 1 commit into from
Jul 27, 2023

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Jul 17, 2023 •
edited
Loading

noreturn attributes have been added to the mingw-w64 headers, mingw-w64/mingw-w64@1690994, meaning that from 11.0.0 onwards, you'll no-longer see -Wreturn-type warnings when using assert(false).

Add -Wno-return-type to the Windows CI, where it should have been all
along, and document why it's required. This can be dropped when we are
using the fixed version of the mingw-w64 headers there.

Drop the -Werror -Wno-return-type special case from our build system.
-Wreturn-type is on by default in Clang and GCC.

The new mingw-w64 header behaviour can be checked on Ubuntu mantic, which ships with 11.0.0, using:

#include <cassert>

int f(){ assert(false); }

int main() {
	return 0;
}

On Mantic (with 11.0.0):

x86_64-w64-mingw32-g++ test.cpp -Wreturn-type
# nada

On Lunar (with 10.0.0):

x86_64-w64-mingw32-g++ test.cpp -Wreturn-type
test.cpp: In function 'int f()':
test.cpp:3:25: warning: no return statement in function returning non-void [-Wreturn-type]
    3 | int f(){ assert(false); }
      |                         ^

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 17, 2023 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK TheCharlatan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25972 (build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set by fanquake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@fanquake fanquake mentioned this pull request Jul 17, 2023
Closed
@fanquake fanquake force-pushed the update_windows_return_type_docs branch from e438a10 to a046ac7 Compare July 17, 2023 15:18
@fanquake
Copy link
Member Author

Remembered why we can't make this change; because the current code was added to our buildsystem as a hack to fix the Windows CI under -Werror, rather than just suppressing the incorrect warning... Took my own advice from 25972 and deleted the check entirely, adding -Wno-return-type to the CI, where it can be dropped once we are using a fixed version of the headers.

@fanquake fanquake changed the title build: document that -Wreturn-type has been fixed upstream (mingw-w64) ci: document that -Wreturn-type has been fixed upstream (mingw-w64) Jul 17, 2023
@maflcko
Copy link
Member

maflcko commented Jul 18, 2023

Looks like this triggers an OOM for some reason, when it shouldn't?

@fanquake
Copy link
Member Author

Looks like this triggers an OOM for some reason, when it shouldn't?

Looks like this is the same bug with overriding cxxflags dropping other flags. Will fix that.

@maflcko
Copy link
Member

maflcko commented Jul 18, 2023

Ok, an alternative would be to embed the -Wno-return-type into the compiler CC and CXX env vars, like I did for the valgrind task.

@fanquake
ci: document that -Wreturn-type has been fixed upstream (Windows)
08eb5f1 
`noreturn` attributes have been added to the mingw-w64 headers, meaning
that from 11.0.0 onwards, you'll no-longer see `-Wreturn-type` warnings
when using assert(false):
mingw-w64/mingw-w64@1690994.

Add -Wno-return-type to the Windows CI, where is should have been all
along, and document why it's required. This can be dropped when we are
using the fixed version of the mingw-w64 headers there.

Drop the -Werror -Wno-return-type special case from our build system.
-Wreturn-type is on by default in Clang and GCC.
@fanquake fanquake force-pushed the update_windows_return_type_docs branch from a046ac7 to 08eb5f1 Compare July 18, 2023 13:32
@fanquake
Copy link
Member Author

Ok. Done with less embedding. Only difference between this and master is s/-Wno-error=return-type/-Wno-return-type/.

i.e master https://cirrus-ci.com/task/6747293471211520?logs=ci#L1367 vs this PR https://cirrus-ci.com/task/5094810491551744?logs=ci#L983.

@DrahtBot DrahtBot mentioned this pull request Jul 19, 2023
Merged
@maflcko
Copy link
Member

maflcko commented Jul 19, 2023

Given that the config will be bumped to gcc-12 anyway (#27897), how hard would it be to bump mingw in guix and CI, and at the same time drop this completely?

@fanquake
Copy link
Member Author

how hard would it be to bump mingw in guix

I have done the bump in Guix: https://lists.gnu.org/archive/html/guix-patches/2023-07/msg00159.html, so we'll have that once the time-machine is bumped.

@maflcko
Copy link
Member

maflcko commented Jul 19, 2023

once the time-machine is bumped.

Doesn't this break other builds? So I was wondering if the time machine could be bumped (for mingw), but the Linux tasks be kept as-is for now?

@maflcko
Copy link
Member

maflcko commented Jul 19, 2023

(I mean, obviously my preference would be to use clang for Windows cross-builds, because mingw doesn't seem to be used by any large/relevant software project anymore, so using it at all is becoming increasingly fragile, but doing another bump shouldn't hurt too much for now)

@fanquake
Copy link
Member Author

So I was wondering if the time machine could be bumped (for mingw), but the Linux tasks be kept as-is for now?

We don't currently have an easy way to use separate time-machines for differents HOSTS, and I'm not sure if that's something we'd want to do. The general time-machine bump is only blocked on one last issue, so that should be done shortly.

(I mean, obviously my preference would be to use clang for Windows cross-builds,

I wouldn't be opposed to that. Although note that in that case we'd still be using mingw-w64, for the Windows headers/runtime etc, swapping out a GCC toolchain for an LLVM/Clang toolchain, so we'd still have to deal with issues like these in either case.

In this PR, I'd really just like to remove the single -Werror special case, and put it into the CI config. Note that moving these things out of the build system also means they wont be (incorrectly) ported over to CMake.

@theuni
Copy link
Member

theuni commented Jul 20, 2023 •
edited
Loading

Makes sense to me. It seems a bit premature maybe, but I agree with the goal of not carrying this type of baggage over to CMake.

Edit: removal would be premature if the flag was necessary for building. But as this arguably never should've been there in the first place, I suppose that's not relevant.

TheCharlatan
TheCharlatan approved these changes Jul 27, 2023
View reviewed changes
Copy link
Contributor

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

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

ACK 08eb5f1

I'm fine with the warnings being printed until the mingw toolchain is rolled out everywhere.

Guix build:

find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum                                                                                                                                                                        
daeec3e7ea34ddf28200d95d96e3295cadfc48fba9d95234880f7c95ec2b56c7  guix-build-08eb5f1b67e2/output/dist-archive/bitcoin-08eb5f1b67e2.tar.gz
3fda74de33a0e2dccbb74db3766a628135c0f0de7f4ad07afaed82050a2a0a1c  guix-build-08eb5f1b67e2/output/x86_64-w64-mingw32/SHA256SUMS.part
ce48e7bb01ed7d2d580a5ac629b515c13974fbfa68496cd902f76f55fae1b6d1  guix-build-08eb5f1b67e2/output/x86_64-w64-mingw32/bitcoin-08eb5f1b67e2-win64-debug.zip
6a7df9551fd286d12676e35af0b082eac731de7e1a4121be321f5d1f20529cdc  guix-build-08eb5f1b67e2/output/x86_64-w64-mingw32/bitcoin-08eb5f1b67e2-win64-setup-unsigned.exe
ac21d932789a1c9bfbd3072d61b8aa4bcdfa4ff97918a7d78320614257538787  guix-build-08eb5f1b67e2/output/x86_64-w64-mingw32/bitcoin-08eb5f1b67e2-win64-unsigned.tar.gz
e57e2ded1b225c6038346348e9b5b9480d75c391329697b5206ed73e586bada9  guix-build-08eb5f1b67e2/output/x86_64-w64-mingw32/bitcoin-08eb5f1b67e2-win64.zip

@fanquake fanquake merged commit 04f66ce into bitcoin:master Jul 27, 2023
@fanquake fanquake deleted the update_windows_return_type_docs branch July 27, 2023 10:22
@hebasto hebasto mentioned this pull request Jul 28, 2023
Merged
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 9, 2023
@fanquake
Merge bitcoin#28092: ci: document that -Wreturn-type has been fixed u…
f688841 
…pstream…
@bitcoin bitcoin locked and limited conversation to collaborators Jul 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@TheCharlatan TheCharlatan TheCharlatan approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Build system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fanquake @DrahtBot @maflcko @theuni @TheCharlatan