On Thu, Sep 11, 2014 at 12:20 PM, Eitan Eliahu <elia...@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:shet...@nicira.com]
> Sent: Thursday, September 11, 2014 10:46 AM
> To: Eitan Eliahu
> Cc: dev@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 <elia...@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:shet...@nicira.com]
>> Sent: Thursday, September 11, 2014 8:30 AM
>> To: Eitan Eliahu
>> Cc: dev@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 <elia...@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=yTvML
>> 8OxA42Jb6ViHe7fUXbvPVOYDPVq87w43doxtlY%3D%0A&m=4G2z2MXJbEYppIl73yQrveJ
>> TD3D0erYuXZJkQ%2BTj8Lo%3D%0A&s=adcbafd1e3da873656b20a37dc68f6b7d2de3c6
>> 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-boun...@openvswitch.org] On Behalf Of
>>> Gurucharan Shetty
>>> Sent: Wednesday, September 10, 2014 12:56 PM
>>> To: dev@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 <gshe...@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@openvswitch.org
>>> https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mai
>>> l
>>> man/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=yTvML8OxA42Jb6
>>> V
>>> iHe7fUXbvPVOYDPVq87w43doxtlY%3D%0A&m=vhaaiwGg2o4fdzo1Y4GXNiuZqtaokSkx
>>> a
>>> JCi51lfBLo%3D%0A&s=916b857d57af38bd52b9bf30cb0dda65b6f0ae970620dcd280
>>> 0
>>> b619fe86a4dee
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
