[ovs-dev] [PATCH 3/3] Avoid uninitialized variable warnings with OBJECT_OFFSETOF() in MSVC.
Gurucharan Shetty
shettyg at nicira.com
Thu Sep 11 17:46:01 UTC 2014
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/en-us/library/2c8f766e.aspx&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=yTvML8OxA42Jb6ViHe7fUXbvPVOYDPVq87w43doxtlY%3D%0A&m=4G2z2MXJbEYppIl73yQrveJTD3D0erYuXZJkQ%2BTj8Lo%3D%0A&s=adcbafd1e3da873656b20a37dc68f6b7d2de3c66db71c7d7fc7e33ff11e8d26e
>
> 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/mail
>> man/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=yTvML8OxA42Jb6V
>> iHe7fUXbvPVOYDPVq87w43doxtlY%3D%0A&m=vhaaiwGg2o4fdzo1Y4GXNiuZqtaokSkxa
>> JCi51lfBLo%3D%0A&s=916b857d57af38bd52b9bf30cb0dda65b6f0ae970620dcd2800
>> b619fe86a4dee
More information about the dev
mailing list