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

Fix building with GCC 8+ #3345

Merged
merged 1 commit into from
Oct 27, 2019
Merged

Fix building with GCC 8+ #3345

merged 1 commit into from
Oct 27, 2019

Conversation

SeanTAllen
Copy link
Member

@SeanTAllen SeanTAllen commented Oct 26, 2019

Starting with GCC 8, they added a new warning stringops-truncation. It
is well intentioned but, seems prone to a lot of false positives.

It found several false positives in the Pony codebase and because we treat
all warnings as errors, you can't build ponyc with GCC 8+.

This commit switches from using strncpy to memcpy. They are in the end, the
same operation except a difference in return type. However, with the new
gcc option, memcpy will not complain about not copying the null terminator.

Fixes #3313

Starting with GCC 8, they added a new warning `stringops-truncation`. It
is well intentioned but, seems prone to a lot of false positives.

It found several false positives in the Pony codebase and because we treat
all warnings as errors, you can't build ponyc with GCC 8+.

This commit switches from using strncpy to memcpy. They are in the end, the
same operation except a difference in return type. However, with the new
gcc option, memcpy will not complain about not copying the null terminator.

Fixes #3313
@SeanTAllen SeanTAllen requested a review from jemc October 26, 2019 22:37
@SeanTAllen SeanTAllen added the do not merge This PR should not be merged at this time label Oct 26, 2019
@SeanTAllen
Copy link
Member Author

For reasons that I don't understand, I was unable to reproduce #3313 with GCC 8.3 on my Ubuntu install. I've asked @stronny to verify that this fixes the issue for them. This is good for review, but I've put a "DO NOT MERGE" on until we get some independent verification it fixes the issue.

@dipinhora
Copy link
Contributor

reproduces error:

docker run --rm -it fedora sh -c 'dnf install -y git gcc-c++ make zlib-devel llvm3.9-devel ncurses-devel  libatomic which findutils && git clone https://github.com/ponylang/ponyc && cd ponyc && LLVM_CONFIG=/usr/lib64/llvm3.9/bin/llvm-config make -j`nproc`'

confirms fix:

docker run --rm -it fedora sh -c 'dnf install -y git gcc-c++ make zlib-devel llvm3.9-devel ncurses-devel  libatomic which findutils && git clone https://github.com/ponylang/ponyc && cd ponyc && git checkout 3313 && LLVM_CONFIG=/usr/lib64/llvm3.9/bin/llvm-config make -j`nproc`'

@SeanTAllen
Copy link
Member Author

Excellent. Thanks, @dipinhora.

@SeanTAllen SeanTAllen removed the do not merge This PR should not be merged at this time label Oct 27, 2019
@SeanTAllen SeanTAllen merged commit 2c3865a into master Oct 27, 2019
@SeanTAllen SeanTAllen deleted the 3313 branch October 27, 2019 00:10
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.

Unable to create Fedora releases due to compilation error
2 participants