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

[RFC] Use GNULIB's compiler warning code #378

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
4 participants
@fidencio
Copy link
Contributor

commented Sep 9, 2017

This is the 3rd tentative to have this patch reviewed. For more references, please, see: PR #50.

So, I've re-worked those patches a little bit and here is the time difference when running reconfing with the patches:

real	0m26.047s
user	0m21.318s
sys	0m4.635s

And now without:

real	0m25.565s
user	0m20.696s
sys	0m4.433s

This patch set is rebased on top of PR #377.

I really would appreciate if someone could review and give their opinion.

The reason this PR was blocked is because this time difference has been considered a "performance issue".

@jhrozek , could you take a look on this?

@fidencio fidencio force-pushed the fidencio:wip/manywarnings branch 4 times, most recently from 4623e3d to 592ecb7 Sep 9, 2017

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2017

The only two patches which have to be reviewed as part of this PR are:

  • CMOCKA: Silence a false-positive "deference of null pointer" warning
  • BUILD: Make use of GNULIB's compiler warning code

The patches which this PR has been rebased atop are here because without then our CI would fail.

@fidencio fidencio force-pushed the fidencio:wip/manywarnings branch from 592ecb7 to fe1c055 Sep 11, 2017

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2017

I'm seeing a build failure with these patches on my machine:

/home/remote/jhrozek/devel/sssd/src/util/crypto/nss/nss_sha512crypt.c: In function ‘sha512_crypt_r’:
/home/remote/jhrozek/devel/sssd/src/util/crypto/nss/nss_sha512crypt.c:71:12: error: stack protector not protecting local variables: variable length buffer [-Werror=stack-protector]
 static int sha512_crypt_r(const char *key,
            ^~~~~~~~~~~~~~
cc1: all warnings being treated as errors
Makefile:17864: recipe for target 'src/util/crypto/nss/libsss_crypt_la-nss_sha512crypt.lo' failed
@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2017

btw as I said in PR #371 since there are fixes developed independently, maybe the patches that are also included from PR #371 should be dropped from this PR so that we can merge both.

@jhrozek jhrozek self-assigned this Sep 12, 2017

new_end->next = NULL;
if (new_end) {
new_end->next = NULL;
}

This comment has been minimized.

Copy link
@jhrozek

jhrozek Sep 12, 2017

Contributor

Wouldn't it make sense to return an error if new_end is NULL? Or is it OK to just ignore that?

@@ -100,7 +100,6 @@
/* If INI_PARSE_IGNORE_NON_KVP is not defined, use 0 (no effect) */
#ifndef INI_PARSE_IGNORE_NON_KVP
#define INI_PARSE_IGNORE_NON_KVP 0
#warning INI_PARSE_IGNORE_NON_KVP not defined.

This comment has been minimized.

Copy link
@jhrozek

jhrozek Sep 12, 2017

Contributor

@mzidek-rh does RHEL-6 ship with the newer ding-libs API? If yes, I guess this is OK, otherwise we should keep the warning..

*/
if (recent_dom_info != NULL) {
recent_dom_info->next = NULL;
}

This comment has been minimized.

Copy link
@jhrozek

jhrozek Sep 12, 2017

Contributor

Similar to the resolv patch, I think it's better to fail explicitly than silently skip even in case of a false positive.

@jhrozek
Copy link
Contributor

left a comment

I would prefer explicit failure in case some check fails and we need to check with @mzidek-rh about removing the INI warning.

@mzidek-rh

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2017

I did not review all the patches, but I do not like the removal of the INI warning. I guess we can not treat warnings as errors for all platforms. It is easy to forget that you need new ding-libs for some of the features to work properly in SSSD and if someone compiles the code for EL6 (scratch builds) this warning is a valuable piece of information. I explicitly grep for it when do some scratch builds to verify if the new ding-libs was available during compile time (not just run time).

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2017

Okay. I'll re-work this PR.

About merging this PR and the #371, I'm okay with that. But I'd prefer dropping the patches from there than from here (as those patches here are at least 6 months older than that ones).

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2017

btw there are more warnings on RHEL-6:

cache_req_data.o `test -f 'src/responder/common/cache_req/cache_req_data.c' || echo '/var/lib/jenkins/workspace/ci/label/rhel6/'`src/responder/common/cache_req/cache_req_data.c
cc1: warning: command line option "-Wenum-compare" is valid for C++/ObjC++ but not for C
cc1: warning: command line option "-Wenum-compare" is valid for C++/ObjC++ but not for C
cc1: warnings being treated as errors
/var/lib/jenkins/workspace/ci/label/rhel6/src/responder/common/cache_req/cache_req_result.c: In function 'cache_req_add_result':
/var/lib/jenkins/workspace/ci/label/rhel6/src/responder/common/cache_req/cache_req_result.c:35: error: declaration of 'index' shadows a global declaration [-Wshadow]
/usr/include/string.h:489: error: shadowed declaration is here [-Wshadow]
@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2017

@fidencio fidencio force-pushed the fidencio:wip/manywarnings branch from fe1c055 to 7f77508 Sep 20, 2017

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2017

@jhrozek, in order to keep this PR moving ... I've decided to do not warn in case of stack-protector.
I'd like to hear from you whether you want a bug filled for this (as it's happening in a few pieces of our code).

I've submit the code to our internal CI and I'll post the link as soon as I get the results.

This PR is rebased atop of #377.

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2017

@pbrezina

This comment has been minimized.

Copy link
Member

commented Sep 22, 2017

@fidencio I'm not Jakub but just file a ticket for those warnings and lets move on with this one. It's been opened a year ago, you deserve it :-)

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2017

@fidencio I'm sorry this stalled, but I guess the PR needs a rebase atop master, right? At least the fixes for the warnings are present in master now..

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2017

Also, I'd like to open at least one bug to fix some of the warnings we've been ignoring so far.

fidencio added some commits Nov 10, 2016

CI: Disable _FORTIFY_SOURCE when building with -O0
This patch is based on cabc051.

Our coverage build uses -O0 but the macro _FORTIFY_SOURCE requires to be
compiled with optimization.

Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
CMOCKA: Silence a false-positive "deference of null pointer" warning
It's needed in case GNULIB's manywarnings patch gets accepted, but it's
clearly a false-positive.

Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
BUILD: Make use of GNULIB's compiler warning code
As GNULIB has the 'manywarnings' module, which basically turns on every
GCC warning, let's make use of it. We can easily blacklist the warnings
we cannot cope with, but the main goal should be to have enabled every
possible GCC warning.

When new GCC warnings are created the 'manywarnings' file can be
refreshed from upstream GNULIB.

Compilation with -Werror is explicitly disabled for RHEL6 in our CI, as
a lot of warnings show up there due to old dependencies or even as
false-positives.

Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>

@fidencio fidencio force-pushed the fidencio:wip/manywarnings branch from 7f77508 to ac54a2e Dec 14, 2017

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2017

@pbrezina, @jhrozek: This PR has been updated and is ready to be reviewed & possibly merged.

I've opened two issues related to this PR: https://pagure.io/SSSD/sssd/issue/3604 and https://pagure.io/SSSD/sssd/issue/3606

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2017

I've fired a CI build and I'm waiting for its results.

@fidencio

This comment has been minimized.

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2018

I've decided to close this PR, there are more important things to be worked on/reviewed.

@fidencio fidencio added the Rejected label Apr 11, 2018

@fidencio fidencio closed this Apr 11, 2018

@pbrezina

This comment has been minimized.

Copy link
Member

commented Apr 12, 2018

I'm sorry this got stalled, does it need any rebasing? If you rebase it one more time, I will review it immediately.

@pbrezina

This comment has been minimized.

Copy link
Member

commented Apr 12, 2018

Same for Amit's patch.

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2018

@pbrezina, thanks for the offer but there's no need, sincerely. :-)

There's more important work to be done/reviewed at the moment and keeping the PR opened didn't make sense.

In the future, in case someone has interest on having this merged, we can get back and talk about it. For now, I'd just keep it closed and forget about this!

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.