[RFC] Use GNULIB's compiler warning code by fidencio · Pull Request #378 · SSSD/sssd

fidencio · 2017-09-09T11:46:20Z

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 · 2017-09-09T13:58:05Z

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.

jhrozek · 2017-09-12T09:42:56Z

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 · 2017-09-12T09:44:28Z

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 · 2017-09-12T09:51:13Z

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


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

jhrozek · 2017-09-12T09:54:42Z

 /* 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.


@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..

jhrozek · 2017-09-12T09:55:43Z

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


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

jhrozek

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

mzidek-gh · 2017-09-12T10:25:02Z

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 · 2017-09-12T10:34:38Z

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 · 2017-09-12T19:36:03Z

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 · 2017-09-12T19:54:49Z

On Tue, Sep 12, 2017 at 9:36 PM, Jakub Hrozek ***@***.***> wrote: 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] — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#378 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAG4ek0NUboT94sK1M5Tln3Fu3VUaSegks5sht0jgaJpZM4PR-L3> .

This one is covered by PR #371. In case @lslebodn agrees I can merge PR #371 and PR #377 and then rebase this PR atop of that one.

fidencio · 2017-09-20T07:43:36Z

@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 · 2017-09-20T08:39:33Z

CI: http://vm-058-233.${abc}/logs/job/77/86/summary.html

pbrezina · 2017-09-22T09:23:38Z

@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 · 2017-09-27T18:59:27Z

@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 · 2017-09-27T21:00:50Z

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

It's needed in case GNULIB's manywarnings patch gets accepted, but it's clearly a false-positive. Signed-off-by: Fabiano Fidêncio <[email protected]>

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 <[email protected]>

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 <[email protected]>

fidencio · 2017-12-14T15:15:25Z

@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 · 2017-12-14T15:16:18Z

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

fidencio · 2017-12-14T18:16:10Z

CI: http://vm-031.abc.idm.lab.eng.brq.redhat.com/logs/job/82/70/summary.html

fidencio · 2018-04-11T16:27:27Z

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

pbrezina · 2018-04-12T07:40:43Z

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

pbrezina · 2018-04-12T07:41:09Z

Same for Amit's patch.

fidencio · 2018-04-12T08:28:53Z

@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!

fidencio force-pushed the wip/manywarnings branch 4 times, most recently from 4623e3d to 592ecb7 Compare September 9, 2017 13:55

fidencio force-pushed the wip/manywarnings branch from 592ecb7 to fe1c055 Compare September 11, 2017 07:28

jhrozek self-assigned this Sep 12, 2017

jhrozek added the Changes requested label Sep 12, 2017

jhrozek reviewed Sep 12, 2017

View reviewed changes

jhrozek suggested changes Sep 12, 2017

View reviewed changes

fidencio force-pushed the wip/manywarnings branch from fe1c055 to 7f77508 Compare September 20, 2017 06:50

fidencio removed the Changes requested label Sep 20, 2017

fidencio mentioned this pull request Sep 25, 2017

This patch set fixes a bunch warnings #377

Closed

fidencio added 3 commits December 14, 2017 12:00

CMOCKA: Silence a false-positive "deference of null pointer" warning

b30a71e

It's needed in case GNULIB's manywarnings patch gets accepted, but it's clearly a false-positive. Signed-off-by: Fabiano Fidêncio <[email protected]>

CI: Disable _FORTIFY_SOURCE when building with -O0

28ce4a8

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 <[email protected]>

fidencio force-pushed the wip/manywarnings branch from 7f77508 to ac54a2e Compare December 14, 2017 15:14

fidencio mentioned this pull request Dec 18, 2017

gcc warnings: Getting rid of -Wsuggest-attribute=noreturn #481

Closed

fidencio mentioned this pull request Jan 2, 2018

SDAP: skip builtin AD groups in sdap_save_grpmem() #484

Closed

fidencio mentioned this pull request Feb 6, 2018

Fix/suppress few gcc8 warnings #508

Closed

fidencio added the Rejected label Apr 11, 2018

fidencio closed this Apr 11, 2018

pbrezina mentioned this pull request Apr 28, 2020

Fail to build upstream code on RHEL-7 when using the GNULIB's compiler warnings as errors pbrezina/sssd-test#3591

Closed

sssd-bot mentioned this pull request May 2, 2020

Fail to build upstream code on RHEL-7 when using the GNULIB's compiler warnings as errors #4629

Closed

Uh oh!

Conversation

fidencio commented Sep 9, 2017

Uh oh!

fidencio commented Sep 9, 2017

Uh oh!

jhrozek commented Sep 12, 2017

Uh oh!

jhrozek commented Sep 12, 2017

Uh oh!

jhrozek Sep 12, 2017

Choose a reason for hiding this comment

Uh oh!

jhrozek Sep 12, 2017

Choose a reason for hiding this comment

Uh oh!

jhrozek Sep 12, 2017

Choose a reason for hiding this comment

Uh oh!

jhrozek left a comment

Choose a reason for hiding this comment

Uh oh!

mzidek-gh commented Sep 12, 2017

Uh oh!

fidencio commented Sep 12, 2017

Uh oh!

jhrozek commented Sep 12, 2017

Uh oh!

fidencio commented Sep 12, 2017 via email

Uh oh!

fidencio commented Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fidencio commented Sep 20, 2017

Uh oh!

pbrezina commented Sep 22, 2017

Uh oh!

jhrozek commented Sep 27, 2017

Uh oh!

fidencio commented Sep 27, 2017

Uh oh!

fidencio commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fidencio commented Dec 14, 2017

Uh oh!

fidencio commented Dec 14, 2017

Uh oh!

fidencio commented Apr 11, 2018

Uh oh!

pbrezina commented Apr 12, 2018

Uh oh!

pbrezina commented Apr 12, 2018

Uh oh!

fidencio commented Apr 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fidencio commented Sep 20, 2017 •

edited

Loading

fidencio commented Dec 14, 2017 •

edited

Loading