[SSSD] [PATCH] Suppress warning maybe-uninitialized
Lukas Slebodnik
lslebodn at redhat.com
Fri Oct 3 08:21:04 UTC 2014
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 at 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
More information about the sssd-devel
mailing list