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

[hail] Add -Wextra to CXXFLAGS #5354

Merged
merged 15 commits into from Feb 19, 2019

Conversation

Projects
None yet
4 participants
@danking
Copy link
Collaborator

commented Feb 14, 2019

Open for debate, but I found that clang++ doesn't warn about signed/unsigned comparisons nor a misuse of a random generator without these, but g++ does. (g++ is used in the build image).

Stacked on #5331

@danking

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 14, 2019

Random draw came up @tpoterba. I think @catoverdrive, @patrick-schultz, @chrisvittal, and @cseed may have feelings about this.

@cseed

This comment has been minimized.

Copy link
Collaborator

commented Feb 14, 2019

I like -Werror and generally more warnings. I think pedantic is slightly too strict (unless you're using the GNU -std or whatever) as the generated C++ code is using the statement expression GNU extension.

@danking danking changed the title [hail] Add -Werror and -Wpedantic to CXXFLAGS [hail] Add -Wextra and -Wpedantic to CXXFLAGS Feb 14, 2019

@danking

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 14, 2019

Whoops sorry, I meant I added -Wextra. I'll remove -Wpedantic though.

@chrisvittal
Copy link
Collaborator

left a comment

I'm having issues with using this Makefile with these changes.

Some of it is warnings present in g++ version 8.2.1, which I will follow up on, so we can fix them. This other one is more fundamental to the Makefile

gcc and clang handle -MM in different ways, causing failures with g++, as g++ -MM -MG ibs.cpp does not create the simd/simd.h dependency that will then cause $(LIBSIMDPP) target to be run, thus leading to simd/simd.h not being found when building build/ibs.o.

$ g++ -MM -MG ibs.cpp 2&> /dev/null
ibs.o: ibs.cpp ibs.h
$ clang++ -MM -MG ibs.cpp 2&> /dev/null
ibs.o: ibs.cpp ibs.h simdpp/simd.h

I believe this has to do with g++ treating any #include <header.h> as system headers, and thus not in the output of -MM.

There are two ways that I have found to fix this:

  1. Use -M in the Makefile, thus depending on ALL the system headers, (which really isn't so bad) and may automatically cause rebuilds with libc or compiler updates (a good thing in my mind).
  2. This patch on ibs.h:
diff --git a/hail/src/main/c/ibs.h b/hail/src/main/c/ibs.h
index c815bcdf0..68ced642d 100644
--- a/hail/src/main/c/ibs.h
+++ b/hail/src/main/c/ibs.h
@@ -14,7 +14,7 @@
 #endif
 #endif // HAIL_OVERRIDE_ARCH
 
-#include <simdpp/simd.h>
+#include "simdpp/simd.h"
 #include <inttypes.h>
 
 using namespace simdpp;
$(BUILD)/unit-tests.o: testutils/unit-tests.cpp
mkdir -p $(@D)

This comment has been minimized.

Copy link
@chrisvittal

chrisvittal Feb 15, 2019

Collaborator

Can we suppress all of these mkdirs? They don't really add anything to the rule. If a mkdir -p fails (really unlikely) then the rule will still fail.

To suppress the output, use @ in front of the rule.

MAKEFLAGS += --no-builtin-rules
.PHONY: all clean debug prebuilt test
.SUFFIXES:
.DEFAULT: all

This comment has been minimized.

Copy link
@chrisvittal

chrisvittal Feb 15, 2019

Collaborator

At least as of version 4.2.1 of GNU make, .DEFAULT doesn't indicate the default target, to do that you need to set the variable .DEFAULT_GOAL, like .DEFAULT_GOAL := all

Documentation on .DEFAULT_GOAL is here

According to the make manual, .DEFAULT does the following:

.DEFAULT

    The recipe specified for .DEFAULT is used for any target for which no rules
    are found (either explicit rules or implicit rules). See Last Resort. If
    a .DEFAULT recipe is specified, every file mentioned as a prerequisite, but
    not as a target in a rule, will have that recipe executed on its behalf.
    See Implicit Rule Search Algorithm. 

As such, when I run make in this directory. It tries to make the first target simd/simd.h

@chrisvittal

This comment has been minimized.

Copy link
Collaborator

commented Feb 15, 2019

Re, my review above, the PR builder uses

# g++ --version
g++ (Debian 6.3.0-18+deb9u1) 6.3.0 20170516
Copyright (C) 2016 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Whereas my machine uses:

$ g++ --version
g++ (Ubuntu 8.2.0-7ubuntu1) 8.2.0
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

So there was a change in behavior between GCC versions. 🤷‍♂ EDIT: Or not. The tests passing is an artifact of the exact order commands are executed in the CI build, rather than the makefile working with older versions of make and g++.

It should still work for all versions we would reasonably expect people to use.

@danking

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 15, 2019

What’s the deal with the ordering? I want to eliminate implicit dependencies

@danking danking force-pushed the danking:new-c-makefile2 branch from fe94493 to eb97d45 Feb 15, 2019

fixed in other pr

@danking danking changed the title [hail] Add -Wextra and -Wpedantic to CXXFLAGS [hail] Add -Wextra to CXXFLAGS Feb 15, 2019

@danking danking force-pushed the danking:new-c-makefile2 branch from 3381389 to 41066c0 Feb 15, 2019

JNIEnv* env,
jobject /*thisJ*/,
JNIEnv*,
jobject,

This comment has been minimized.

Copy link
@danking

danking Feb 15, 2019

Author Collaborator

gcc doesn't like that JNIEnv* env is unused, so I remove it's name. Looks like that was already done for the other two, but with commented name. I removed the comment. @catoverdrive, do you know why these methods have unused arguments?

@danking danking assigned chrisvittal and unassigned tpoterba Feb 15, 2019

@danking danking force-pushed the danking:new-c-makefile2 branch from 7caf515 to 3827551 Feb 15, 2019

@danking

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 15, 2019

I should say, -Wpedantic is pretty nice actually, it correctly flags davies.cpp as a bit of a mess.

@danking danking force-pushed the danking:new-c-makefile2 branch from 97e063d to 047858b Feb 15, 2019

@@ -344,28 +344,27 @@ real DaviesAlgo::qf(real* lb1, real* nc1, int* n1, int r1, real sigma, real c1,

extern "C"
EXPORT
real qfWrapper(real* lb1, real* nc1, int* n1, int r1, real sigma, real c1,
real qfWrapper(real* lb1, real* nc1, int* dof, int r1, real sigma, real c1,

This comment has been minimized.

Copy link
@danking

danking Feb 15, 2019

Author Collaborator

see the caller Skat.scala to understand why the transformations here are the correct ones. I think it was pedantic that actually caught all these unused variables and non-standard c.

@danking

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 15, 2019

@chrisvittal if you're good with this please approve :)

@chrisvittal
Copy link
Collaborator

left a comment

There is one more warning in davies.cpp. GCC warns that we may clobber qfval, and we assign to it anyways later in the function. I'm not sure of the semantics of setjmp and longjmp, but the following patch supresses the warning.

diff --git a/hail/src/main/c/davies.cpp b/hail/src/main/c/davies.cpp
index 23d152d31..0aa60b1fe 100644
--- a/hail/src/main/c/davies.cpp
+++ b/hail/src/main/c/davies.cpp
@@ -238,7 +238,7 @@ output:
 {
   int j, nj, nt, ntm;  real acc1, almx, xlim, xnt, xntm;
   real utx, tausq, sd, intv, intv1, x, up, un, d1, d2, lj, ncj;
-  real qfval = -1.0;
+  real qfval;
   static int rats[]={1,2,4,8};
   
   if (setjmp(env) != 0) { *ifault=4; goto endofproc; }
@danking

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 15, 2019

Ah shit, that's in -Wextra? @chrisvittal do you know if that patch also passes the tests? I was worried that change would break something.

@chrisvittal

This comment has been minimized.

Copy link
Collaborator

commented Feb 15, 2019

I do not. Haven’t tried yet. I will shortly.

@danking

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 15, 2019

I think you can push to this branch directly, then CI will test for you.

@danking

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 15, 2019

I can check when I get in (ETA 1430)

chrisvittal added some commits Feb 15, 2019

Fix linker command line argument order
Library order matters and can cause frustrating symbol not found errors
depending on how the compiler invokes the linker. Putting the -ldl
argument last fixes this.
@chrisvittal

This comment has been minimized.

Copy link
Collaborator

commented Feb 15, 2019

Waiting on CI, but I have now done enough work on this that I probably shouldn't give final approval?

@chrisvittal

This comment has been minimized.

Copy link
Collaborator

commented Feb 15, 2019

Ugh, discovered a problem with race conditions surrounding the test.cpp build path. Can cause failures with a naked make -jN test on a clean directory. Fixing.

@danking

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 15, 2019

Ok clearly I need to install g++.

@danking

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 15, 2019

@chrisvittal I think you I and cotton are the ones best suited to review these though, so 🤷‍♀️

danking added some commits Feb 15, 2019

# be set explicitly via HAIL_OVERRIDE_WIDTH
$(BUILD)/ibs.d: CXXFLAGS += -DHAIL_OVERRIDE_WIDTH=4 -DNUMBER_OF_GENOTYPES_PER_ROW=256
$(BUILD)/ibs.o: CXXFLAGS += -DNUMBER_OF_GENOTYPES_PER_ROW=256

This comment has been minimized.

Copy link
@danking

danking Feb 15, 2019

Author Collaborator

The tests indicate this should just be set when we're compiling the functional-tests (the tests for ibs.cpp) so I moved it to the appropriate location down below.

@danking

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 15, 2019

Ok I think this works and is simple. @chrisvittal let me know what you think

danking added some commits Feb 15, 2019

@danking

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 15, 2019

crushed it

@chrisvittal
Copy link
Collaborator

left a comment

I think that we finally got it. There's one issue regarding linker argument order that differs between my machine and the PR builder, but that can be a follow up.

Need to request changes again :(

@chrisvittal
Copy link
Collaborator

left a comment

We need to fix the tests. We've talked about a special test version of ibs.o with -DNUMBER_OF_GENOTYPES_PER_ROW=256, but also before this we were building ibs.o with the define anyways. Why don't we just add -DNUMBER_OF_GENOTYPES_PER_ROW=256 to CXXFLAGS?

@@ -163,6 +154,7 @@ endif
echo "CXX is $(CXX)"
-$(CXX) --version

$(BUILD)/functional-tests: CXXFLAGS += -DNUMBER_OF_GENOTYPES_PER_ROW=256

This comment has been minimized.

Copy link
@chrisvittal

chrisvittal Feb 19, 2019

Collaborator

This doesn't quite do it. For the tests, we need to have both object files built with the flag enabled. This worked with make test from clean because all targets required for functional-tests were built with -DNUMBER_OF_GENOTYPES_PER_ROW=256. But it fails with make; make test because ibs.o will not get built with the define.

@danking

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 19, 2019

We can't add it to CFLAGS because Hail expects it to be set to 1024. We use 256 in the tests to make the test cases easier to write.

fix
@danking

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 19, 2019

This is a special case target anyway for an old way of testing so I special cased it even further.

@danking danking merged commit 9e4417b into hail-is:master Feb 19, 2019

1 check passed

hail-ci-0-1 successful build
Details

danking added a commit to danking/hail that referenced this pull request Mar 1, 2019

[hail] fix SKAT, which was broken by hail-is#5354
I did not rebuild the shared libraries. I do not understand why
the tests did not fail. I will look into that separately. This
should unblock TJ.

danking added a commit that referenced this pull request Mar 2, 2019

[hail] fix SKAT, which was broken by #5354 (#5508)
I did not rebuild the shared libraries. I do not understand why
the tests did not fail. I will look into that separately. This
should unblock TJ.

@danking danking referenced this pull request Mar 8, 2019

Closed

SKAT segfaults #5565

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.