Compile Software with gcc instead of clang #1129

jonathanlew · 2020-01-20T05:26:41Z

Please fill out the following before requesting review on this PR

Description

Use native gcc to compile code instead of external clang package.

Replaced magic_enum because magic_enum only works on gcc 9
Some tweaks to some tests
Some compiler error fixes

Testing Done

Resolved Issues

resolves #1124

Length Justification

Review Checklist

It is the reviewers responsibility to also make sure every item here has been covered

Function & Class comments: All function definitions (usually in the .h file) should have a javadoc style comment at the start of them. For examples, see the functions defined in thunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.
Remove all commented out code
Remove extra print statements: for example, those just used for testing
Resolve all TODO's: All TODO (or similar) statements should either be completed or associated with a github issue
Justify drops in code coverage: If this PR results in a non-trivial drop in code coverage (a bot should post a coverage diagram as a comment), please justify why we can't test the code that's not covered.

jonathanlew

Few interesting bits for reviewers to look at

jonathanlew · 2020-01-20T05:36:55Z

src/cc_toolchain/wrapper/k8_gcc-k8_gcc

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+exec /usr/bin/x86_64-linux-gnu-gcc -Wl,--no-as-needed "$@"


@garethellis0 can you do better than this?

Ok, I don't immediately see a better way 😢. I'm fine with this as long as we leave a comment and open an issue with https://github.com/bazelbuild/bazel asking if this is the correct behavior. I can do make the issue or you can, just lemme know!

drafting an email to [email protected] according to this message:

ATTENTION! Please read and follow:

if this is a question about how to build / test / query / deploy using Bazel, or a discussion starter, send it to [email protected]

if this is a bug or feature request, fill the form below as best as you can.

src/software/ai/passing/pass_generator_test.cpp

garethellis0

Looking good! Will try to find a way to put this compiler flags in the toolchain now........

src/cc_toolchain/BUILD

src/cc_toolchain/cc_toolchain_config.bzl

src/software/util/enum_string_macro/BUILD

MathewMacDougall

Overall looking good. Still seems to be some clang stuff floating around in files that got renamed like
src/cc_toolchain/wrapper/clang-profdata → src/cc_toolchain/wrapper/k8_gcc-profdata
src/cc_toolchain/wrapper/clang-strip → src/cc_toolchain/wrapper/k8_gcc-strip

MathewMacDougall · 2020-01-21T06:11:01Z

src/cc_toolchain/BUILD

        "/usr/include/",
        "/usr/local/include/",
+        "/usr/lib/gcc/c++/7.4.0/include/",


Just to be clear, we explicitly have the include directory for 7.4.0 because we still depend on libs / code built with that version of gcc, so this is where we need to look to find it? Do we have any guarantee this folder exists?

I think ubuntu 18.04 has gcc 7.4.0 preinstalled

Perhaps we should install gcc 7.4.0 explicitly in the setup scripts @garethellis0 ? I don't reall want to have stuff just magically assumed.

Fine with me, installing gcc-7 should do the trick I think.

src/cc_toolchain/wrapper/k8_gcc-cov

MathewMacDougall · 2020-01-21T06:18:50Z

src/external/k8_gcc.BUILD

+
+filegroup(
+    name = "k8_gcc_libs",
+    srcs = glob(["usr/lib/gcc/x86_64-linux-gnu/7.4.0/*.a"]),


Similar to my previous comment, should this really all be specifying 7.4.0?

ubuntu 18.04 should have 7.4.0

I think the question here is can we make this a bit more general, ie. usr/lib/gcc/x86_64-linux-gnu/*/*.a or maybe usr/lib/gcc/x86_64-linux-gnu/*/*.a, but I think that could cause issues if there are multiple versions of gcc installed. perhaps we could make this a filegroup in the arm_gcc.BUILD file, just so that all the version-specific stuff is in one place?

Yes I'm mostly just concerned about us

Expecting a specific version of gcc that we do nothing to verify exists

"hardcoding" the version in multiple places

I just want this to be easy to upgrade and change in the future, and not cause errors with non-obvious messages if someone without the right setup tries to compile

I'm not sure how to filegroup them any better. I think I can reduce changes required to update just src/external/k8_gcc.BUILD

I think the issue here is maybe more in the naming. Could we rename this file to linux_gcc_7_4_0, because that's how specific it is? Similairly, everything in this file should probably be changed from k8_gcc to linux_gcc_7_4_0.

This way if we ever go to upgrade the compiler version or even support multiple platforms, it's clear what bits are specific to what.

should be good now

MathewMacDougall · 2020-01-31T18:51:19Z

Only remaining comment for me is gcc versioning, otherwise nothing more to add beyond existing comments (and CI needs to pass).

garethellis0 · 2020-02-04T17:19:46Z

Looks like there was a small (but somewhat nasty) bug where we were relying on just reading random memory here. Opened a quick PR to this branch to fix: https://github.com/jonathanlew/Software/pull/1/files

…d-behavior Added Check To Make Sure Shot Optional Is Not std::nullopt

jonathanlew · 2020-02-05T21:20:15Z

Added back the clang-format binary so CI should pass now

jonathanlew · 2020-02-05T21:50:45Z

@garethellis0 @MathewMacDougall re-review?

garethellis0 · 2020-02-05T23:25:33Z

Looks like CI failed due to a network issue. Re-running now.

garethellis0

Main comments from my end here are just around what we call k8_gcc vs linux_gcc_7_4_0.

Also one comment left above from an earlier review.

Overall looking good! This will definitely be an improvement.

src/cc_toolchain/BUILD

garethellis0 · 2020-02-05T23:31:22Z

src/external/k8_gcc.BUILD

+
+filegroup(
+    name = "k8_gcc_libs",
+    srcs = glob(["usr/lib/gcc/x86_64-linux-gnu/7.4.0/*.a"]),


I think the issue here is maybe more in the naming. Could we rename this file to linux_gcc_7_4_0, because that's how specific it is? Similairly, everything in this file should probably be changed from k8_gcc to linux_gcc_7_4_0.

This way if we ever go to upgrade the compiler version or even support multiple platforms, it's clear what bits are specific to what.

MathewMacDougall

Nothing major to add beyond Gareth's comments. Can we also change the setup scripts to ensure we install gcc 7.4.0?

src/software/geom/rectangle.h

jonathanlew · 2020-02-08T15:27:41Z

Ready for re-review

garethellis0

Nice job getting comments resolved! Just one left (at the very top) and then this is good to go from my end!

jonathanlew · 2020-02-08T21:51:23Z

Nice job getting comments resolved! Just one left (at the very top) and then this is good to go from my end!

Will email [email protected] re: -Wl,--no-as-needed in linux_gcc-gcc

.travis.yml

Co-Authored-By: Gareth Ellis <[email protected]>

MathewMacDougall · 2020-02-15T00:41:13Z

Seems like this is good to go then? @garethellis0 is everything good from your end?

garethellis0 · 2020-02-15T17:54:23Z

Still waiting on code coverage here..... not sure why it's not running, I think Jon is going to open a new PR to see if that fixes it?

jonathanlew · 2020-02-20T10:07:42Z

Will close and reopen to try to fix coverage

Jonathan Lew added 12 commits January 16, 2020 00:35

add gcc targets

42dcaba

some fixes

3946938

fixed more issues, will now build non-linking targets

a62dbc1

fix linker issue

4f880c5

fix constexpre problems

af679f5

fix lm linking

ef902e0

use stlib flags

f48624c

add no-as-needed to fix linking issue

67c11af

replace magic enum with macros

f06f1fb

Make tests pass

b5ba37a

Remove clang

404730e

revert coverage change

1acc1d0

jonathanlew requested review from akhilveeraghanta and kelvinkoon January 20, 2020 05:26

jonathanlew requested review from garethellis0 and MathewMacDougall as code owners January 20, 2020 05:26

fix formatting in refbox_data enum

58aea26

jonathanlew commented Jan 20, 2020

View reviewed changes

remove misc comments

625b79b

garethellis0 requested changes Jan 21, 2020

View reviewed changes

src/cc_toolchain/BUILD Outdated Show resolved Hide resolved

src/cc_toolchain/cc_toolchain_config.bzl Show resolved Hide resolved

src/software/util/enum_string_macro/BUILD Outdated Show resolved Hide resolved

MathewMacDougall requested changes Jan 21, 2020

View reviewed changes

Jonathan Lew added 5 commits January 31, 2020 12:01

Merge remote-tracking branch 'upstream/master' into gcc_compiler

b647d16

Merge remote-tracking branch 'upstream/master' into gcc_compiler

bd892e3

Clean up coverage files

36f1972

rename enum string macro to prinable enum macro

06aceb6

re-enable pass generator tests

3a5ecf7

jonathanlew requested review from garethellis0 and MathewMacDougall January 31, 2020 15:23

Added Check To Make Sure Shot Optional Is Not std::nullopt

1b1c100

jonathanlew and others added 4 commits February 4, 2020 21:19

Merge pull request #1 from garethellis0/gareth/fix-shot-eval-undefine…

81a21ad

…d-behavior Added Check To Make Sure Shot Optional Is Not std::nullopt

Merge remote-tracking branch 'upstream/master' into gcc_compiler

075929b

rename k8_gcc to linux_gcc

2799959

Fix formatting script to work without llvm-clang

e763325

clean up

ae4c5c4

garethellis0 requested changes Feb 5, 2020

View reviewed changes

MathewMacDougall requested changes Feb 6, 2020

View reviewed changes

src/software/geom/rectangle.h Outdated Show resolved Hide resolved

Jonathan Lew added 4 commits February 8, 2020 12:19

Merge remote-tracking branch 'upstream/master' into gcc_compiler

7448612

Name 7.4.0 specific files and globs more clearly

d83d1d1

Rename wrappers to linux

7cd6fa2

clean up more files and add gcc-7 to setup_software script

4d1a9ca

jonathanlew requested review from garethellis0 and MathewMacDougall February 8, 2020 15:27

jonathanlew mentioned this pull request Feb 8, 2020

Switch From Clang To GCC To Fix Code Coverage Issues #989

Closed

5 tasks

garethellis0 reviewed Feb 8, 2020

View reviewed changes

jonathanlew requested a review from garethellis0 February 8, 2020 21:51

Add comment for the no-as-needed flag

6207303

garethellis0 reviewed Feb 9, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

jonathanlew and others added 2 commits February 9, 2020 20:39

Update .travis.yml

82ebcfb

Co-Authored-By: Gareth Ellis <[email protected]>

Fix CI

4739c2a

jonathanlew closed this Feb 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compile Software with gcc instead of clang #1129

Compile Software with gcc instead of clang #1129

jonathanlew commented Jan 20, 2020 •

edited

Loading

jonathanlew left a comment

jonathanlew Jan 20, 2020

garethellis0 Jan 21, 2020

jonathanlew Feb 8, 2020

garethellis0 left a comment

MathewMacDougall left a comment

MathewMacDougall Jan 21, 2020

jonathanlew Jan 31, 2020

MathewMacDougall Jan 31, 2020

garethellis0 Feb 1, 2020

jonathanlew Feb 8, 2020

MathewMacDougall Jan 21, 2020

jonathanlew Jan 31, 2020

garethellis0 Feb 4, 2020

MathewMacDougall Feb 4, 2020

jonathanlew Feb 5, 2020

garethellis0 Feb 5, 2020

jonathanlew Feb 8, 2020

MathewMacDougall commented Jan 31, 2020

garethellis0 commented Feb 4, 2020

jonathanlew commented Feb 5, 2020

jonathanlew commented Feb 5, 2020

garethellis0 commented Feb 5, 2020

garethellis0 left a comment

garethellis0 Feb 5, 2020

MathewMacDougall left a comment

jonathanlew commented Feb 8, 2020

garethellis0 left a comment

jonathanlew commented Feb 8, 2020

MathewMacDougall commented Feb 15, 2020

garethellis0 commented Feb 15, 2020

jonathanlew commented Feb 20, 2020

Compile Software with gcc instead of clang #1129

Compile Software with gcc instead of clang #1129

Conversation

jonathanlew commented Jan 20, 2020 • edited Loading

Please fill out the following before requesting review on this PR

Description

Testing Done

Resolved Issues

Length Justification

Review Checklist

jonathanlew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garethellis0 left a comment

Choose a reason for hiding this comment

MathewMacDougall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MathewMacDougall commented Jan 31, 2020

garethellis0 commented Feb 4, 2020

jonathanlew commented Feb 5, 2020

jonathanlew commented Feb 5, 2020

garethellis0 commented Feb 5, 2020

garethellis0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MathewMacDougall left a comment

Choose a reason for hiding this comment

jonathanlew commented Feb 8, 2020

garethellis0 left a comment

Choose a reason for hiding this comment

jonathanlew commented Feb 8, 2020

MathewMacDougall commented Feb 15, 2020

garethellis0 commented Feb 15, 2020

jonathanlew commented Feb 20, 2020

jonathanlew commented Jan 20, 2020 •

edited

Loading