Skip to content

runtime: use std::max_align_t over max_align_t (NFC) #17068

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

Merged
merged 1 commit into from
Jun 8, 2018

Conversation

compnerd
Copy link
Member

@compnerd compnerd commented Jun 8, 2018

MSVC does not provide a full C99 environment. Use the C++ equivalent
spelling for the value. This allows the build to succeed on Windows.

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves SR-NNNN.

MSVC does not provide a full C99 environment.  Use the C++ equivalent
spelling for the value.  This allows the build to succeed on Windows.
@compnerd
Copy link
Member Author

compnerd commented Jun 8, 2018

@swift-ci please test and merge

@swift-ci swift-ci merged commit bd5e7c4 into swiftlang:master Jun 8, 2018
@xedin
Copy link
Contributor

xedin commented Jun 9, 2018

@compnerd Looks like this broke Ubuntu 14.04 bot - https://ci.swift.org/job/oss-swift-incremental-RA-linux-ubuntu-14_04/4486/

@compnerd
Copy link
Member Author

compnerd commented Jun 10, 2018

Weird, std::max_align_t is part of C++11, it should be present. http://en.cppreference.com/w/cpp/types/max_align_t indicates that the included header should define it. Perhaps there is another header that can be included as a workaround.

@compnerd compnerd deleted the maxalign branch June 10, 2018 00:01
@millenomi
Copy link
Contributor

@compnerd I think you need to #include <cstddef> instead of including the C header #include "stddef.h".

@millenomi
Copy link
Contributor

Can we revert, or correct this with a new PR that is tested on 14.04?

@xedin
Copy link
Contributor

xedin commented Jun 11, 2018

Maybe #17101 is going to fix it?

@compnerd
Copy link
Member Author

@millenomi - trying to setup a 14.04 environment to look into this. We should certainly look into correcting this :-)

The interesting thing is that the change does include cstddef. Perhaps @xedin's patch will fix it by removing the inclusion of the C stddef.h include.

@xedin
Copy link
Contributor

xedin commented Jun 11, 2018

Looks like my changes didn't help but it's good to have them anyway...

@jrose-apple
Copy link
Contributor

There's a stddef.h in include/swift/Runtime/Portability.h too. Is that (transitively) included?

@xedin
Copy link
Contributor

xedin commented Jun 12, 2018

Maybe, I've opened #17126 to deal with that as well...

@compnerd
Copy link
Member Author

I'm still trying to build in a VM :-(

@rudkx
Copy link
Contributor

rudkx commented Jun 12, 2018

#17126 does not appear to have fixed this, so I've opened #17139 to revert the original change until this can be sorted out.

@compnerd
Copy link
Member Author

compnerd commented Jun 12, 2018

I didn't expect building against 14.04 to be so challenging. Unfortunately, this change is needed to build for Windows, so both states are less than ideal. The solution seems to be that we need to special case the 14.04 build. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56019 indicates that GCC 4.8 has a bug in the C++11 standard library implementation and is missing the definition of std::max_align_t. This has been fixed in libstdc++ which is distributed as part of GCC 4.9. I'll upload a workaround for this.

@rudkx
Copy link
Contributor

rudkx commented Jun 12, 2018

I'm happy to hold off on merging the revert if you can land something relatively quickly that resolves the issue, but we shouldn't let this keep the CI red for much longer.

@compnerd
Copy link
Member Author

@rudkx, @xedin I think that #17140 should repair the 14.04 build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants