ehlo,
There is a warning with some version of gcc (4.9.1, 4.4.7). Warning is not visible with gcc 4.8 In my opinion, variable "a" should be initialized every time if "maps" is not NULL.
src/providers/ldap/sdap.c: In function 'sdap_parse_entry': src/providers/ldap/sdap.c:481:56: warning: 'a' may be used uninitialized in this function [-Wmaybe-uninitialized] for (ai = a; ai < attrs_num; ai++) { ^ src/providers/ldap/sdap.c:307:9: note: 'a' was declared here int a, i, ret, ai; ^ CCLD libsss_ldap_common.la
You can also see warnin in koji build on fedora 21 https://kojipkgs.fedoraproject.org//packages/sssd/1.12.1/2.fc21/data/logs/x8... If you have better idea how to fix this warning I will be glad to review your patch.
LS
On Thu, 2 Oct 2014 19:33:02 +0200 Lukas Slebodnik lslebodn@redhat.com wrote:
ehlo,
There is a warning with some version of gcc (4.9.1, 4.4.7). Warning is not visible with gcc 4.8 In my opinion, variable "a" should be initialized every time if "maps" is not NULL.
I agree that "a" should be intialized every time, in fact I do not agree with your patch where you set it to be a volatile, it is not.
The main issue here is that the name and way this variable is used is confusing.
A better way would be to define a locally in the first loop, and in the function define a better name initialized to a negative value.
the name should probably be called something like base_attr_idx and intialized as:
int base_attr_idx = 0;
and if the first part of the code finds the base_attr it is looking for it would simply assing a to it (note we do away with "a" completely).
if (map) { for (i = 1; i < attrs_num; i++) { ... } if (i < attrs_num) { store = true; base_attr_idx = i; ... } }
then the second for loop where you get the error would be change to: for (ai = base_attr_idx; ai > 0 && ai < attrs_num; ai++) {
Simo.
src/providers/ldap/sdap.c: In function 'sdap_parse_entry': src/providers/ldap/sdap.c:481:56: warning: 'a' may be used uninitialized in this function [-Wmaybe-uninitialized] for (ai = a; ai < attrs_num; ai++) { ^ src/providers/ldap/sdap.c:307:9: note: 'a' was declared here int a, i, ret, ai; ^ CCLD libsss_ldap_common.la
You can also see warnin in koji build on fedora 21 https://kojipkgs.fedoraproject.org//packages/sssd/1.12.1/2.fc21/data/logs/x8... If you have better idea how to fix this warning I will be glad to review your patch.
LS
On (02/10/14 15:12), Simo Sorce wrote:
On Thu, 2 Oct 2014 19:33:02 +0200 Lukas Slebodnik lslebodn@redhat.com wrote:
ehlo,
There is a warning with some version of gcc (4.9.1, 4.4.7). Warning is not visible with gcc 4.8 In my opinion, variable "a" should be initialized every time if "maps" is not NULL.
I agree that "a" should be intialized every time, in fact I do not agree with your patch where you set it to be a volatile, it is not.
The main issue here is that the name and way this variable is used is confusing.
A better way would be to define a locally in the first loop, and in the function define a better name initialized to a negative value.
the name should probably be called something like base_attr_idx and intialized as:
int base_attr_idx = 0;
and if the first part of the code finds the base_attr it is looking for it would simply assing a to it (note we do away with "a" completely).
if (map) { for (i = 1; i < attrs_num; i++) { ... } if (i < attrs_num) { store = true; base_attr_idx = i; ... } }
then the second for loop where you get the error would be change to: for (ai = base_attr_idx; ai > 0 && ai < attrs_num; ai++) {
I suppose you meant similar patch diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 7034b11..6e8b16e 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -305,6 +305,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx, char *str; int lerrno; int a, i, ret, ai; + int base_attr_idx; const char *name; bool store; bool base64; @@ -412,6 +413,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx, /* interesting attr */ if (a < attrs_num) { store = true; + base_attr_idx = a; name = map[a].sys_name; if (strcmp(name, SYSDB_SSH_PUBKEY) == 0) { base64 = true; @@ -478,7 +480,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx, * attrs in case there is a map. Find all that match * and copy the value */ - for (ai = a; ai < attrs_num; ai++) { + for (ai = base_attr_idx; ai < attrs_num; ai++) { /* check if this attr is valid with the chosen * schema */ if (!map[ai].name) continue;
It didn't help. src/providers/ldap/sdap.c: In function 'sdap_parse_entry': src/providers/ldap/sdap.c:308:9: error: 'base_attr_idx' may be used uninitialized in this function [-Werror=maybe-uninitialized] int base_attr_idx; ^ cc1: all warnings being treated as errors (BTW: There is no difference between using a and base_attr_idx)
Any other better solution is welcomed.
LS
On Thu, Oct 02, 2014 at 10:20:56PM +0200, Lukas Slebodnik wrote:
On (02/10/14 15:12), Simo Sorce wrote:
On Thu, 2 Oct 2014 19:33:02 +0200 Lukas Slebodnik lslebodn@redhat.com wrote:
ehlo,
There is a warning with some version of gcc (4.9.1, 4.4.7). Warning is not visible with gcc 4.8 In my opinion, variable "a" should be initialized every time if "maps" is not NULL.
I agree that "a" should be intialized every time, in fact I do not agree with your patch where you set it to be a volatile, it is not.
The main issue here is that the name and way this variable is used is confusing.
A better way would be to define a locally in the first loop, and in the function define a better name initialized to a negative value.
the name should probably be called something like base_attr_idx and intialized as:
int base_attr_idx = 0;
and if the first part of the code finds the base_attr it is looking for it would simply assing a to it (note we do away with "a" completely).
if (map) { for (i = 1; i < attrs_num; i++) { ... } if (i < attrs_num) { store = true; base_attr_idx = i; ... } }
then the second for loop where you get the error would be change to: for (ai = base_attr_idx; ai > 0 && ai < attrs_num; ai++) {
I suppose you meant similar patch diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 7034b11..6e8b16e 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -305,6 +305,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx, char *str; int lerrno; int a, i, ret, ai;
- int base_attr_idx; const char *name; bool store; bool base64;
@@ -412,6 +413,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx, /* interesting attr */ if (a < attrs_num) { store = true;
base_attr_idx = a; name = map[a].sys_name; if (strcmp(name, SYSDB_SSH_PUBKEY) == 0) { base64 = true;@@ -478,7 +480,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx, * attrs in case there is a map. Find all that match * and copy the value */
for (ai = a; ai < attrs_num; ai++) {
for (ai = base_attr_idx; ai < attrs_num; ai++) { /* check if this attr is valid with the chosen * schema */ if (!map[ai].name) continue;It didn't help. src/providers/ldap/sdap.c: In function 'sdap_parse_entry': src/providers/ldap/sdap.c:308:9: error: 'base_attr_idx' may be used uninitialized in this function [-Werror=maybe-uninitialized] int base_attr_idx; ^ cc1: all warnings being treated as errors (BTW: There is no difference between using a and base_attr_idx)
Any other better solution is welcomed.
Just a wild guess. Maybe the compiler is doing some wild optimizations assuming that attrs_num might be 1, 0 or even smaller. Does it help to check for (map && num_attrs > 1) ? (I would try myself, but I do not have the right gcc version at hand)
bye, Sumit
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (02/10/14 22:51), Sumit Bose wrote:
On Thu, Oct 02, 2014 at 10:20:56PM +0200, Lukas Slebodnik wrote:
On (02/10/14 15:12), Simo Sorce wrote:
On Thu, 2 Oct 2014 19:33:02 +0200 Lukas Slebodnik lslebodn@redhat.com wrote:
ehlo,
There is a warning with some version of gcc (4.9.1, 4.4.7). Warning is not visible with gcc 4.8 In my opinion, variable "a" should be initialized every time if "maps" is not NULL.
I agree that "a" should be intialized every time, in fact I do not agree with your patch where you set it to be a volatile, it is not.
The main issue here is that the name and way this variable is used is confusing.
A better way would be to define a locally in the first loop, and in the function define a better name initialized to a negative value.
the name should probably be called something like base_attr_idx and intialized as:
int base_attr_idx = 0;
and if the first part of the code finds the base_attr it is looking for it would simply assing a to it (note we do away with "a" completely).
if (map) { for (i = 1; i < attrs_num; i++) { ... } if (i < attrs_num) { store = true; base_attr_idx = i; ... } }
then the second for loop where you get the error would be change to: for (ai = base_attr_idx; ai > 0 && ai < attrs_num; ai++) {
I suppose you meant similar patch diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 7034b11..6e8b16e 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -305,6 +305,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx, char *str; int lerrno; int a, i, ret, ai;
- int base_attr_idx; const char *name; bool store; bool base64;
@@ -412,6 +413,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx, /* interesting attr */ if (a < attrs_num) { store = true;
base_attr_idx = a; name = map[a].sys_name; if (strcmp(name, SYSDB_SSH_PUBKEY) == 0) { base64 = true;@@ -478,7 +480,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx, * attrs in case there is a map. Find all that match * and copy the value */
for (ai = a; ai < attrs_num; ai++) {
for (ai = base_attr_idx; ai < attrs_num; ai++) { /* check if this attr is valid with the chosen * schema */ if (!map[ai].name) continue;It didn't help. src/providers/ldap/sdap.c: In function 'sdap_parse_entry': src/providers/ldap/sdap.c:308:9: error: 'base_attr_idx' may be used uninitialized in this function [-Werror=maybe-uninitialized] int base_attr_idx; ^ cc1: all warnings being treated as errors (BTW: There is no difference between using a and base_attr_idx)
Any other better solution is welcomed.
Just a wild guess. Maybe the compiler is doing some wild optimizations assuming that attrs_num might be 1, 0 or even smaller. Does it help to check for (map && num_attrs > 1) ? (I would try myself, but I do not have the right gcc version at hand)
It didn't help either.
Compiler do some kind of non obvious optimisation with variable "a" or with simo's proprosed variable "base_attr_idx".
Suppressing optimization with keyword volatile fixes a warning. I would like to have better solution, but I cannot find any.
LS
----- Original Message -----
From: "Lukas Slebodnik" lslebodn@redhat.com To: "Simo Sorce" simo@redhat.com Cc: sssd-devel@lists.fedorahosted.org Sent: Thursday, October 2, 2014 4:20:56 PM Subject: Re: [SSSD] [PATCH] Suppress warning maybe-uninitialized
On (02/10/14 15:12), Simo Sorce wrote:
On Thu, 2 Oct 2014 19:33:02 +0200 Lukas Slebodnik lslebodn@redhat.com wrote:
ehlo,
There is a warning with some version of gcc (4.9.1, 4.4.7). Warning is not visible with gcc 4.8 In my opinion, variable "a" should be initialized every time if "maps" is not NULL.
I agree that "a" should be intialized every time, in fact I do not agree with your patch where you set it to be a volatile, it is not.
The main issue here is that the name and way this variable is used is confusing.
A better way would be to define a locally in the first loop, and in the function define a better name initialized to a negative value.
the name should probably be called something like base_attr_idx and intialized as:
int base_attr_idx = 0;
and if the first part of the code finds the base_attr it is looking for it would simply assing a to it (note we do away with "a" completely).
if (map) { for (i = 1; i < attrs_num; i++) { ... } if (i < attrs_num) { store = true; base_attr_idx = i; ... } }
then the second for loop where you get the error would be change to: for (ai = base_attr_idx; ai > 0 && ai < attrs_num; ai++) {
I suppose you meant similar patch diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 7034b11..6e8b16e 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -305,6 +305,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx, char *str; int lerrno; int a, i, ret, ai;
- int base_attr_idx; const char *name; bool store; bool base64;
@@ -412,6 +413,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx, /* interesting attr */ if (a < attrs_num) { store = true;
base_attr_idx = a; name = map[a].sys_name; if (strcmp(name, SYSDB_SSH_PUBKEY) == 0) { base64 = true;@@ -478,7 +480,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx, * attrs in case there is a map. Find all that match * and copy the value */
for (ai = a; ai < attrs_num; ai++) {
for (ai = base_attr_idx; ai < attrs_num; ai++) { /* check if this attr is valid with the chosen * schema */ if (!map[ai].name) continue;It didn't help. src/providers/ldap/sdap.c: In function 'sdap_parse_entry': src/providers/ldap/sdap.c:308:9: error: 'base_attr_idx' may be used uninitialized in this function [-Werror=maybe-uninitialized] int base_attr_idx; ^ cc1: all warnings being treated as errors (BTW: There is no difference between using a and base_attr_idx)
Any other better solution is welcomed.
You did not follow my advice. There were 2 fundamental changes in my proposal
1. declare and initialize the new variable to 0 int base_attr_idx = 0;
2. in the second for loop add ai > 0 to the condition, otherwise you'll run through even if base_attr_idx was never intentionally set.
With these 2 changes you won't get the warning and the behaviour will be correct.
Additionally, if you want, you can completely drop the 'a' variable and use 'i' everywhere instead.
Simo.
On (03/10/14 09:40), Simo Sorce wrote:
----- Original Message -----
From: "Lukas Slebodnik" lslebodn@redhat.com To: "Simo Sorce" simo@redhat.com Cc: sssd-devel@lists.fedorahosted.org Sent: Thursday, October 2, 2014 4:20:56 PM Subject: Re: [SSSD] [PATCH] Suppress warning maybe-uninitialized
On (02/10/14 15:12), Simo Sorce wrote:
On Thu, 2 Oct 2014 19:33:02 +0200 Lukas Slebodnik lslebodn@redhat.com wrote:
ehlo,
There is a warning with some version of gcc (4.9.1, 4.4.7). Warning is not visible with gcc 4.8 In my opinion, variable "a" should be initialized every time if "maps" is not NULL.
I agree that "a" should be intialized every time, in fact I do not agree with your patch where you set it to be a volatile, it is not.
The main issue here is that the name and way this variable is used is confusing.
A better way would be to define a locally in the first loop, and in the function define a better name initialized to a negative value.
the name should probably be called something like base_attr_idx and intialized as:
int base_attr_idx = 0;
and if the first part of the code finds the base_attr it is looking for it would simply assing a to it (note we do away with "a" completely).
if (map) { for (i = 1; i < attrs_num; i++) { ... } if (i < attrs_num) { store = true; base_attr_idx = i; ... } }
then the second for loop where you get the error would be change to: for (ai = base_attr_idx; ai > 0 && ai < attrs_num; ai++) {
I suppose you meant similar patch diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 7034b11..6e8b16e 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -305,6 +305,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx, char *str; int lerrno; int a, i, ret, ai;
- int base_attr_idx; const char *name; bool store; bool base64;
@@ -412,6 +413,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx, /* interesting attr */ if (a < attrs_num) { store = true;
base_attr_idx = a; name = map[a].sys_name; if (strcmp(name, SYSDB_SSH_PUBKEY) == 0) { base64 = true;@@ -478,7 +480,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx, * attrs in case there is a map. Find all that match * and copy the value */
for (ai = a; ai < attrs_num; ai++) {
for (ai = base_attr_idx; ai < attrs_num; ai++) { /* check if this attr is valid with the chosen * schema */ if (!map[ai].name) continue;It didn't help. src/providers/ldap/sdap.c: In function 'sdap_parse_entry': src/providers/ldap/sdap.c:308:9: error: 'base_attr_idx' may be used uninitialized in this function [-Werror=maybe-uninitialized] int base_attr_idx; ^ cc1: all warnings being treated as errors (BTW: There is no difference between using a and base_attr_idx)
Any other better solution is welcomed.
You did not follow my advice. There were 2 fundamental changes in my proposal
- declare and initialize the new variable to 0
int base_attr_idx = 0;
It would be the same solution as initialising variable "a" to 0, it fixes warning as well. I just want to avoid such solution, because it can hide real problem where variable can be used uninitialized.
- in the second for loop add ai > 0 to the condition, otherwise you'll
run through even if base_attr_idx was never intentionally set.
With these 2 changes you won't get the warning and the behaviour will be correct.
Do we really need the additional condition? In worst case, we will search base_attr in map from beginning. I would like to have as simple change as possible. (don't want to break anything).
Additionally, if you want, you can completely drop the 'a' variable and use 'i' everywhere instead.
yes, will do.
It seems that we will not find better solution than initialize base_attr_idx to 0. If there aren't any objections or better solution I will send patch on Monday.
LS
On (03/10/14 09:40), Simo Sorce wrote:
----- Original Message -----
From: "Lukas Slebodnik" lslebodn@redhat.com To: "Simo Sorce" simo@redhat.com Cc: sssd-devel@lists.fedorahosted.org Sent: Thursday, October 2, 2014 4:20:56 PM Subject: Re: [SSSD] [PATCH] Suppress warning maybe-uninitialized
On (02/10/14 15:12), Simo Sorce wrote:
On Thu, 2 Oct 2014 19:33:02 +0200 Lukas Slebodnik lslebodn@redhat.com wrote:
ehlo,
There is a warning with some version of gcc (4.9.1, 4.4.7). Warning is not visible with gcc 4.8 In my opinion, variable "a" should be initialized every time if "maps" is not NULL.
I agree that "a" should be intialized every time, in fact I do not agree with your patch where you set it to be a volatile, it is not.
The main issue here is that the name and way this variable is used is confusing.
A better way would be to define a locally in the first loop, and in the function define a better name initialized to a negative value.
the name should probably be called something like base_attr_idx and intialized as:
int base_attr_idx = 0;
and if the first part of the code finds the base_attr it is looking for it would simply assing a to it (note we do away with "a" completely).
if (map) { for (i = 1; i < attrs_num; i++) { ... } if (i < attrs_num) { store = true; base_attr_idx = i; ... } }
then the second for loop where you get the error would be change to: for (ai = base_attr_idx; ai > 0 && ai < attrs_num; ai++) {
I suppose you meant similar patch diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 7034b11..6e8b16e 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -305,6 +305,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx, char *str; int lerrno; int a, i, ret, ai;
- int base_attr_idx; const char *name; bool store; bool base64;
@@ -412,6 +413,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx, /* interesting attr */ if (a < attrs_num) { store = true;
base_attr_idx = a; name = map[a].sys_name; if (strcmp(name, SYSDB_SSH_PUBKEY) == 0) { base64 = true;@@ -478,7 +480,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx, * attrs in case there is a map. Find all that match * and copy the value */
for (ai = a; ai < attrs_num; ai++) {
for (ai = base_attr_idx; ai < attrs_num; ai++) { /* check if this attr is valid with the chosen * schema */ if (!map[ai].name) continue;It didn't help. src/providers/ldap/sdap.c: In function 'sdap_parse_entry': src/providers/ldap/sdap.c:308:9: error: 'base_attr_idx' may be used uninitialized in this function [-Werror=maybe-uninitialized] int base_attr_idx; ^ cc1: all warnings being treated as errors (BTW: There is no difference between using a and base_attr_idx)
Any other better solution is welcomed.
You did not follow my advice. There were 2 fundamental changes in my proposal
- declare and initialize the new variable to 0
int base_attr_idx = 0;
- in the second for loop add ai > 0 to the condition, otherwise you'll
run through even if base_attr_idx was never intentionally set.
With these 2 changes you won't get the warning and the behaviour will be correct.
Additionally, if you want, you can completely drop the 'a' variable and use 'i' everywhere instead.
Updated version of patch is attached.
LS
sssd-devel@lists.fedorahosted.org
-
Lukas Slebodnik -
Simo Sorce -
Sumit Bose