[ovs-dev] [PATCH 3/3] Avoid uninitialized variable warnings with OBJECT_OFFSETOF() in MSVC.

Eitan Eliahu eliahue at vmware.com
Thu Sep 11 23:06:08 UTC 2014


Actually, I wanted to push and pop the warning settings. But, the new warning level is taken into effect only after the compiler encounters the open curly brace. This means that we need to push and pop the warning on the conataing macros. This change is not trivial.
I'm still looking for the issue with decltype () operator in the OVS code base.
Please go ahead  with your change as we don't have anything better right now.
Thanks,
Eitan

-----Original Message-----
From: Gurucharan Shetty [mailto:shettyg at nicira.com] 
Sent: Thursday, September 11, 2014 12:44 PM
To: Eitan Eliahu
Cc: dev at openvswitch.org; Gurucharan Shetty
Subject: Re: [ovs-dev] [PATCH 3/3] Avoid uninitialized variable warnings with OBJECT_OFFSETOF() in MSVC.

On Thu, Sep 11, 2014 at 12:20 PM, Eitan Eliahu <eliahue at vmware.com> wrote:
>
> Here is the local warning suppression :
> #define OBJECT_OFFSETOF(OBJECT, MEMBER) __pragma(pack(push, 1)) 
> __pragma(warning(disable:4700)) ((char *)&(OBJECT)->MEMBER - (char 
> *)(OBJECT)) __pragma(pack(pop))

It is possible that I don't understand an implicit meaning in the above line. You are making an alignment change, followed by a warning disable. Which you never restore. Which in effect is a global disable of warning 4700. That is the reason you won't see any warnings with this change.
>
>
> The right way to do it is as follows, but it doesn't work on OVS 
> source base,
>
> struct my_node {
>     int first;
>     int extra_data;
> };
>
> #define OBJECT_OFFSETOF1(OBJECT, MEMBER) \
>     ((LONG)(LONG_PTR)&((decltype(OBJECT))0)->MEMBER)
>
> int _tmain(int argc, _TCHAR* argv[])
> {
>     long l1 = (long)OBJECT_OFFSETOF1(node, extra_data);
>     printf("Number is %d\n", l1);
>     return 0;
> }

From what I have read in msdn documentation, decltype is a C++ construct. I haven't been able to use it successfully with a C compiler.

> Eitan
>
> -----Original Message-----
> From: Gurucharan Shetty [mailto:shettyg at nicira.com]
> Sent: Thursday, September 11, 2014 10:46 AM
> To: Eitan Eliahu
> Cc: dev at openvswitch.org; Gurucharan Shetty
> Subject: Re: [ovs-dev] [PATCH 3/3] Avoid uninitialized variable warnings with OBJECT_OFFSETOF() in MSVC.
>
> On Thu, Sep 11, 2014 at 10:41 AM, Eitan Eliahu <eliahue at vmware.com> wrote:
>> As an alternative I tried to use decltype() operator (which I thing is the right way to do it):
>>
>> #define OBJECT_OFFSETOF(OBJECT, MEMBER) \
>>     ((LONG)(LONG_PTR)&((decltype(OBJECT))0)->MEMBER);
>>
>> This worked correctly in a standalone program I wrote but for some reason any use decltypea s an l-value does not work in OVS source files.
>>
>> Anyway, usage of the warning pragma is fine for now.
> What do you mean when you say that "usage of the warning pragma is fine for now". It does not work.
>
>> Thanks,
>> Eitan
>>
>> -----Original Message-----
>> From: Gurucharan Shetty [mailto:shettyg at nicira.com]
>> Sent: Thursday, September 11, 2014 8:30 AM
>> To: Eitan Eliahu
>> Cc: dev at openvswitch.org; Gurucharan Shetty
>> Subject: Re: [ovs-dev] [PATCH 3/3] Avoid uninitialized variable warnings with OBJECT_OFFSETOF() in MSVC.
>>
>> On Wed, Sep 10, 2014 at 10:20 PM, Eitan Eliahu <eliahue at vmware.com> wrote:
>>> Hi Guru,
>>> You can just suppress the warning by wrapping the code in a pragama warning block:
>>>
>>> #pragma warning( push )
>>> #pragma warning( disable : 4707 )
>>> // Some code
>>> #pragma warning( pop )
>>
>> For this particular case, I think what you suggest is a reasonable approach, if it actually worked (or if you know how to get it working).
>>
>> For e.g., the following diff applied on the tip of master should have solved the problem for us.
>> diff --git a/lib/util.h b/lib/util.h
>> index dc34ee5..951aca8 100644
>> --- a/lib/util.h
>> +++ b/lib/util.h
>> @@ -201,7 +201,10 @@ ovs_prefetch_range(const void *start, size_t size)  #define OBJECT_OFFSETOF(OBJECT, MEMBER) offsetof(typeof(*(OBJECT)), MEMBER)  #else  #define OBJECT_OFFSETOF(OBJECT, MEMBER) \
>> -    ((char *) &(OBJECT)->MEMBER - (char *) (OBJECT))
>> +    __pragma (warning(push)) \
>> +    __pragma (warning(disable:4700)) \
>> +    ((char *) &(OBJECT)->MEMBER - (char *) (OBJECT)) \
>> +    __pragma (warning(pop))
>>  #endif
>>
>>
>> But it doesn't. I think the reason it doesn't is because of the following information in msdn documentation.
>>
>> "For warning numbers in the range 4700-4999, which are the ones associated with code generation, the state of the warning in effect when the compiler encounters the open curly brace of a function will be in effect for the rest of the function. Using the warning pragma in the function to change the state of a warning that has a number larger than 4699 will only take effect after the end of the function. The following example shows the correct placement of warning pragmas to disable a code-generation warning message, and then to restore it."
>>
>> Ref:
>> https://urldefense.proofpoint.com/v1/url?u=http://msdn.microsoft.com/
>> e 
>> n-us/library/2c8f766e.aspx&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=yTvM
>> L 
>> 8OxA42Jb6ViHe7fUXbvPVOYDPVq87w43doxtlY%3D%0A&m=4G2z2MXJbEYppIl73yQrve
>> J
>> TD3D0erYuXZJkQ%2BTj8Lo%3D%0A&s=adcbafd1e3da873656b20a37dc68f6b7d2de3c
>> 6
>> 6db71c7d7fc7e33ff11e8d26e
>>
>> If what the above says is true, it is not really feasible to disable a warning for every function that has a *_FOR_EACH macro call.
>>
>>
>>>
>>> You don't want to initialize to NULL just for suppressing the warning.
>>> Thanks,
>>> Eitan
>>>
>>>
>>> -----Original Message-----
>>> From: dev [mailto:dev-bounces at openvswitch.org] On Behalf Of 
>>> Gurucharan Shetty
>>> Sent: Wednesday, September 10, 2014 12:56 PM
>>> To: dev at openvswitch.org
>>> Cc: Gurucharan Shetty
>>> Subject: [ovs-dev] [PATCH 3/3] Avoid uninitialized variable warnings with OBJECT_OFFSETOF() in MSVC.
>>>
>>> Implementation of OBJECT_OFFSETOF() for non-GNUC compilers like MSVC causes "uninitialized variable" warnings. Since OBJECT_OFFSETOF() is indirectly used through all the *_FOR_EACH() (through ASSIGN_CONTAINER() and  OBJECT_CONTAINING()) macros, the OVS build on Windows gets littered with "uninitialized variable" warnings.
>>> This patch attempts to workaround the problem.
>>>
>>> Signed-off-by: Gurucharan Shetty <gshetty at nicira.com>
>>> ---
>>>  lib/classifier.c |    5 +++--
>>>  lib/classifier.h |    4 ++--
>>>  lib/cmap.h       |    6 +++---
>>>  lib/heap.h       |    2 +-
>>>  lib/hindex.h     |   10 +++++-----
>>>  lib/hmap.h       |   10 +++++-----
>>>  lib/list.h       |   10 +++++-----
>>>  lib/util.h       |    7 +++++++
>>>  8 files changed, 31 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/lib/classifier.c b/lib/classifier.c index
>>> ae03251..ee737a7 100644
>>> --- a/lib/classifier.c
>>> +++ b/lib/classifier.c
>>> @@ -1561,7 +1561,7 @@ find_match_wc(const struct cls_subtable *subtable, const struct flow *flow,
>>>                struct flow_wildcards *wc)  {
>>>      uint32_t basis = 0, hash;
>>> -    struct cls_match *rule;
>>> +    struct cls_match *rule = NULL;
>>>      int i;
>>>      struct range ofs;
>>>
>>> @@ -1767,7 +1767,8 @@ static struct cls_match *  next_rule_in_list__(struct cls_match *rule)
>>>      OVS_NO_THREAD_SAFETY_ANALYSIS
>>>  {
>>> -    struct cls_match *next = OBJECT_CONTAINING(rule->list.next, next, list);
>>> +    struct cls_match *next = NULL;
>>> +    next = OBJECT_CONTAINING(rule->list.next, next, list);
>>>      return next;
>>>  }
>>>
>>> diff --git a/lib/classifier.h b/lib/classifier.h index
>>> b394724..d6ab144 100644
>>> --- a/lib/classifier.h
>>> +++ b/lib/classifier.h
>>> @@ -334,7 +334,7 @@ void cls_cursor_advance(struct cls_cursor *);
>>>  #define CLS_FOR_EACH_TARGET(RULE, MEMBER, CLS, TARGET)                  \
>>>      for (struct cls_cursor cursor__ = cls_cursor_start(CLS, TARGET, false); \
>>>           (cursor__.rule                                                 \
>>> -          ? (ASSIGN_CONTAINER(RULE, cursor__.rule, MEMBER),            \
>>> +          ? (INIT_CONTAINER(RULE, cursor__.rule, MEMBER),               \
>>>               true)                                                      \
>>>            : false);                                                     \
>>>           cls_cursor_advance(&cursor__)) @@ -345,7 +345,7 @@ void cls_cursor_advance(struct cls_cursor *);
>>>  #define CLS_FOR_EACH_TARGET_SAFE(RULE, MEMBER, CLS, TARGET)             \
>>>      for (struct cls_cursor cursor__ = cls_cursor_start(CLS, TARGET, true); \
>>>           (cursor__.rule                                                 \
>>> -          ? (ASSIGN_CONTAINER(RULE, cursor__.rule, MEMBER),            \
>>> +          ? (INIT_CONTAINER(RULE, cursor__.rule, MEMBER),               \
>>>               cls_cursor_advance(&cursor__),                             \
>>>               true)                                                      \
>>>            : false);                                                     \
>>> diff --git a/lib/cmap.h b/lib/cmap.h index 038db6c..793202d 100644
>>> --- a/lib/cmap.h
>>> +++ b/lib/cmap.h
>>> @@ -114,11 +114,11 @@ void cmap_replace(struct cmap *, struct cmap_node *old_node,
>>>   * to change during iteration.  It may be very slightly faster.
>>>   */
>>>  #define CMAP_FOR_EACH_WITH_HASH(NODE, MEMBER, HASH, CMAP)       \
>>> -    for (ASSIGN_CONTAINER(NODE, cmap_find(CMAP, HASH), MEMBER); \
>>> +    for (INIT_CONTAINER(NODE, cmap_find(CMAP, HASH), MEMBER);   \
>>>           (NODE) != OBJECT_CONTAINING(NULL, NODE, MEMBER);       \
>>>           ASSIGN_CONTAINER(NODE, cmap_node_next(&(NODE)->MEMBER), MEMBER))
>>>  #define CMAP_FOR_EACH_WITH_HASH_PROTECTED(NODE, MEMBER, HASH, CMAP)        \
>>> -    for (ASSIGN_CONTAINER(NODE, cmap_find_locked(CMAP, HASH), MEMBER);  \
>>> +    for (INIT_CONTAINER(NODE, cmap_find_locked(CMAP, HASH), MEMBER);    \
>>>           (NODE) != OBJECT_CONTAINING(NULL, NODE, MEMBER);               \
>>>           ASSIGN_CONTAINER(NODE, cmap_node_next_protected(&(NODE)->MEMBER), \
>>>                            MEMBER))
>>> @@ -174,7 +174,7 @@ struct cmap_node *cmap_find_protected(const 
>>> struct cmap *, uint32_t hash);
>>>
>>>  #define CMAP_CURSOR_FOR_EACH__(NODE, CURSOR, MEMBER)    \
>>>      ((CURSOR)->node                                     \
>>> -     ? (ASSIGN_CONTAINER(NODE, (CURSOR)->node, MEMBER), \
>>> +     ? (INIT_CONTAINER(NODE, (CURSOR)->node, MEMBER),   \
>>>          cmap_cursor_advance(CURSOR),                    \
>>>          true)                                           \
>>>       : false)
>>> diff --git a/lib/heap.h b/lib/heap.h index 870f582..8de9ea6 100644
>>> --- a/lib/heap.h
>>> +++ b/lib/heap.h
>>> @@ -68,7 +68,7 @@ void heap_rebuild(struct heap *);
>>>   * element. */
>>>  #define HEAP_FOR_EACH(NODE, MEMBER, HEAP)                           \
>>>      for (((HEAP)->n > 0                                             \
>>> -          ? ASSIGN_CONTAINER(NODE, (HEAP)->array[1], MEMBER)        \
>>> +          ? INIT_CONTAINER(NODE, (HEAP)->array[1], MEMBER)          \
>>>            : ((NODE) = NULL, (void) 0));                               \
>>>           (NODE) != NULL;                                            \
>>>           ((NODE)->MEMBER.idx < (HEAP)->n                            \
>>> diff --git a/lib/hindex.h b/lib/hindex.h index 631fd48..416da05
>>> 100644
>>> --- a/lib/hindex.h
>>> +++ b/lib/hindex.h
>>> @@ -128,7 +128,7 @@ void hindex_remove(struct hindex *, struct hindex_node *);
>>>   * Evaluates HASH only once.
>>>   */
>>>  #define HINDEX_FOR_EACH_WITH_HASH(NODE, MEMBER, HASH, HINDEX)               \
>>> -    for (ASSIGN_CONTAINER(NODE, hindex_node_with_hash(HINDEX, HASH), MEMBER); \
>>> +    for (INIT_CONTAINER(NODE, hindex_node_with_hash(HINDEX, HASH), 
>>> + MEMBER); \
>>>           NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER);                     \
>>>           ASSIGN_CONTAINER(NODE, (NODE)->MEMBER.s, MEMBER))
>>>
>>> @@ -149,16 +149,16 @@ hindex_node_with_hash(const struct hindex 
>>> *hindex, size_t hash)
>>>
>>>  /* Iterates through every node in HINDEX. */
>>>  #define HINDEX_FOR_EACH(NODE, MEMBER, HINDEX)                           \
>>> -    for (ASSIGN_CONTAINER(NODE, hindex_first(HINDEX), MEMBER);          \
>>> +    for (INIT_CONTAINER(NODE, hindex_first(HINDEX), MEMBER);            \
>>>           NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER);                 \
>>>           ASSIGN_CONTAINER(NODE, hindex_next(HINDEX, 
>>> &(NODE)->MEMBER),
>>> MEMBER))
>>>
>>>  /* Safe when NODE may be freed (not needed when NODE may be removed from the
>>>   * hash index but its members remain accessible and intact). */
>>> -#define HINDEX_FOR_EACH_SAFE(NODE, NEXT, MEMBER, HINDEX)                \
>>> -    for (ASSIGN_CONTAINER(NODE, hindex_first(HINDEX), MEMBER);          \
>>> +#define HINDEX_FOR_EACH_SAFE(NODE, NEXT, MEMBER, HINDEX)              \
>>> +    for (INIT_CONTAINER(NODE, hindex_first(HINDEX), MEMBER);          \
>>>           (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)                 \
>>> -          ? ASSIGN_CONTAINER(NEXT, hindex_next(HINDEX, &(NODE)->MEMBER), MEMBER), 1 \
>>> +          ? INIT_CONTAINER(NEXT, hindex_next(HINDEX, 
>>> + &(NODE)->MEMBER), MEMBER), 1 \
>>>            : 0);                                                         \
>>>           (NODE) = (NEXT))
>>>
>>> diff --git a/lib/hmap.h b/lib/hmap.h index 9fb83d5..5fcb7a2 100644
>>> --- a/lib/hmap.h
>>> +++ b/lib/hmap.h
>>> @@ -126,12 +126,12 @@ struct hmap_node *hmap_random_node(const struct hmap *);
>>>   * HASH is only evaluated once.
>>>   */
>>>  #define HMAP_FOR_EACH_WITH_HASH(NODE, MEMBER, HASH, HMAP)               \
>>> -    for (ASSIGN_CONTAINER(NODE, hmap_first_with_hash(HMAP, HASH), MEMBER); \
>>> +    for (INIT_CONTAINER(NODE, hmap_first_with_hash(HMAP, HASH), 
>>> + MEMBER); \
>>>           NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER);                  \
>>>           ASSIGN_CONTAINER(NODE, hmap_next_with_hash(&(NODE)->MEMBER),   \
>>>                            MEMBER))
>>>  #define HMAP_FOR_EACH_IN_BUCKET(NODE, MEMBER, HASH, HMAP)               \
>>> -    for (ASSIGN_CONTAINER(NODE, hmap_first_in_bucket(HMAP, HASH), MEMBER); \
>>> +    for (INIT_CONTAINER(NODE, hmap_first_in_bucket(HMAP, HASH), 
>>> + MEMBER); \
>>>           NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER);                  \
>>>           ASSIGN_CONTAINER(NODE,
>>> hmap_next_in_bucket(&(NODE)->MEMBER),
>>> MEMBER))
>>>
>>> @@ -148,16 +148,16 @@ bool hmap_contains(const struct hmap *, const 
>>> struct hmap_node *);
>>>
>>>  /* Iterates through every node in HMAP. */
>>>  #define HMAP_FOR_EACH(NODE, MEMBER, HMAP)                               \
>>> -    for (ASSIGN_CONTAINER(NODE, hmap_first(HMAP), MEMBER);              \
>>> +    for (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER);                \
>>>           NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER);                  \
>>>           ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER),
>>> MEMBER))
>>>
>>>  /* Safe when NODE may be freed (not needed when NODE may be removed from the
>>>   * hash map but its members remain accessible and intact). */
>>>  #define HMAP_FOR_EACH_SAFE(NODE, NEXT, MEMBER, HMAP)                    \
>>> -    for (ASSIGN_CONTAINER(NODE, hmap_first(HMAP), MEMBER);              \
>>> +    for (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER);                \
>>>           (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)                  \
>>> -          ? ASSIGN_CONTAINER(NEXT, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER), 1 \
>>> +          ? INIT_CONTAINER(NEXT, hmap_next(HMAP, &(NODE)->MEMBER), 
>>> + MEMBER), 1 \
>>>            : 0);                                                         \
>>>           (NODE) = (NEXT))
>>>
>>> diff --git a/lib/list.h b/lib/list.h index 0da082e..ef6a9db 100644
>>> --- a/lib/list.h
>>> +++ b/lib/list.h
>>> @@ -58,15 +58,15 @@ bool list_is_singleton(const struct list *); 
>>> bool list_is_short(const struct list *);
>>>
>>>  #define LIST_FOR_EACH(ITER, MEMBER, LIST)                               \
>>> -    for (ASSIGN_CONTAINER(ITER, (LIST)->next, MEMBER);                  \
>>> +    for (INIT_CONTAINER(ITER, (LIST)->next, MEMBER);                    \
>>>           &(ITER)->MEMBER != (LIST);                                     \
>>>           ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.next, MEMBER))
>>>  #define LIST_FOR_EACH_CONTINUE(ITER, MEMBER, LIST)                      \
>>> -    for (ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.next, MEMBER);           \
>>> +    for (INIT_CONTAINER(ITER, (ITER)->MEMBER.next, MEMBER);             \
>>>           &(ITER)->MEMBER != (LIST);                                     \
>>>           ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.next, MEMBER))
>>>  #define LIST_FOR_EACH_REVERSE(ITER, MEMBER, LIST)                       \
>>> -    for (ASSIGN_CONTAINER(ITER, (LIST)->prev, MEMBER);                  \
>>> +    for (INIT_CONTAINER(ITER, (LIST)->prev, MEMBER);                    \
>>>           &(ITER)->MEMBER != (LIST);                                     \
>>>           ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.prev, MEMBER))
>>>  #define LIST_FOR_EACH_REVERSE_CONTINUE(ITER, MEMBER, LIST)              \
>>> @@ -74,9 +74,9 @@ bool list_is_short(const struct list *);
>>>           &(ITER)->MEMBER != (LIST);                                     \
>>>           ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.prev, MEMBER))
>>>  #define LIST_FOR_EACH_SAFE(ITER, NEXT, MEMBER, LIST)               \
>>> -    for (ASSIGN_CONTAINER(ITER, (LIST)->next, MEMBER);             \
>>> +    for (INIT_CONTAINER(ITER, (LIST)->next, MEMBER);               \
>>>           (&(ITER)->MEMBER != (LIST)                                \
>>> -          ? ASSIGN_CONTAINER(NEXT, (ITER)->MEMBER.next, MEMBER), 1 \
>>> +          ? INIT_CONTAINER(NEXT, (ITER)->MEMBER.next, MEMBER), 1   \
>>>            : 0);                                                    \
>>>           (ITER) = (NEXT))
>>>
>>> diff --git a/lib/util.h b/lib/util.h index 261b4b3..a2c6ee9 100644
>>> --- a/lib/util.h
>>> +++ b/lib/util.h
>>> @@ -228,6 +228,13 @@ ovs_prefetch_range(const void *start, size_t size)  #define ASSIGN_CONTAINER(OBJECT, POINTER, MEMBER) \
>>>      ((OBJECT) = OBJECT_CONTAINING(POINTER, OBJECT, MEMBER), (void)
>>> 0)
>>>
>>> +/* As explained in the comment above OBJECT_OFFSETOF(), non-GNUC 
>>> +compilers
>>> + * like MSVC will complain about un-initialized variables if OBJECT
>>> + * hasn't already been initialized. To prevent such warnings,
>>> +INIT_CONTAINER()
>>> + * can be used as a wrapper around ASSIGN_CONTAINER. */ #define 
>>> +INIT_CONTAINER(OBJECT, POINTER, MEMBER) \
>>> +    ((OBJECT) = NULL, ASSIGN_CONTAINER(OBJECT, POINTER, MEMBER))
>>> +
>>>  /* Given ATTR, and TYPE, cast the ATTR to TYPE by first casting ATTR to
>>>   * (void *). This is to suppress the alignment warning issued by 
>>> clang. */  #define ALIGNED_CAST(TYPE, ATTR) ((TYPE) (void *) (ATTR))
>>> --
>>> 1.7.9.5
>>>
>>> _______________________________________________
>>> dev mailing list
>>> dev at openvswitch.org
>>> https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/ma
>>> i
>>> l
>>> man/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=yTvML8OxA42Jb
>>> 6
>>> V
>>> iHe7fUXbvPVOYDPVq87w43doxtlY%3D%0A&m=vhaaiwGg2o4fdzo1Y4GXNiuZqtaokSk
>>> x
>>> a
>>> JCi51lfBLo%3D%0A&s=916b857d57af38bd52b9bf30cb0dda65b6f0ae970620dcd28
>>> 0
>>> 0
>>> b619fe86a4dee


More information about the dev mailing list