diff mbox

get rid of not sized array at middle of the struct

Message ID 1401384287-22415-1-git-send-email-maxim.uvarov@linaro.org
State New
Headers show

Commit Message

Maxim Uvarov May 29, 2014, 5:24 p.m. UTC 
Compilation with c99 finds warning:
 field 'buf_hdr' with variable sized type
 'odp_buffer_hdr_t' (aka 'struct odp_buffer_hdr_t')
 not at the end of a struct or class is a GNU extension

 This happens because odp_buffer_hdr_t struct has payload[]
at the and this struct is a part of other struct:

typedef struct odp_buffer_chunk_hdr_t {
	odp_buffer_hdr_t   buf_hdr; <- payload[] here
	odp_buffer_chunk_t chunk;
} odp_buffer_chunk_hdr_t;

This patch is propose to fix launchpad bug 1274577.

Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
---
 platform/linux-generic/include/odp_buffer_internal.h | 6 ++----
 platform/linux-generic/source/odp_buffer_pool.c      | 2 +-
 2 files changed, 3 insertions(+), 5 deletions(-)

Comments

Anders Roxell June 3, 2014, 7:41 a.m. UTC | #1 
On 2014-05-29 21:24, Maxim Uvarov wrote:
> Compilation with c99 finds warning:
>  field 'buf_hdr' with variable sized type
>  'odp_buffer_hdr_t' (aka 'struct odp_buffer_hdr_t')
>  not at the end of a struct or class is a GNU extension
> 
>  This happens because odp_buffer_hdr_t struct has payload[]
> at the and this struct is a part of other struct:
> 
> typedef struct odp_buffer_chunk_hdr_t {
> 	odp_buffer_hdr_t   buf_hdr; <- payload[] here
> 	odp_buffer_chunk_t chunk;
> } odp_buffer_chunk_hdr_t;
> 
> This patch is propose to fix launchpad bug 1274577.
> 
> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> ---
>  platform/linux-generic/include/odp_buffer_internal.h | 6 ++----
>  platform/linux-generic/source/odp_buffer_pool.c      | 2 +-
>  2 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/platform/linux-generic/include/odp_buffer_internal.h b/platform/linux-generic/include/odp_buffer_internal.h
> index a1a6b4e..f2d409b 100644
> --- a/platform/linux-generic/include/odp_buffer_internal.h
> +++ b/platform/linux-generic/include/odp_buffer_internal.h
> @@ -92,12 +92,10 @@ typedef struct odp_buffer_hdr_t {
>  	int                      type;       /* type of next header */
>  	odp_buffer_pool_t        pool;       /* buffer pool */
>  
> -	uint8_t                  payload[];  /* next header or data */
> +	uint8_t 		 pad[32];    /* to pass check_align() */

Why do we chose 32?

- Anders

> +	/* next header or data just after this sctuct */
>  } odp_buffer_hdr_t;
>  
> -ODP_ASSERT(sizeof(odp_buffer_hdr_t) == ODP_OFFSETOF(odp_buffer_hdr_t, payload),
> -	   ODP_BUFFER_HDR_T__SIZE_ERROR);
> -
>  
>  typedef struct odp_buffer_chunk_hdr_t {
>  	odp_buffer_hdr_t   buf_hdr;
> diff --git a/platform/linux-generic/source/odp_buffer_pool.c b/platform/linux-generic/source/odp_buffer_pool.c
> index 90214ba..0687bcb 100644
> --- a/platform/linux-generic/source/odp_buffer_pool.c
> +++ b/platform/linux-generic/source/odp_buffer_pool.c
> @@ -206,7 +206,7 @@ static void fill_hdr(void *ptr, pool_entry_t *pool, uint32_t index,
>  {
>  	odp_buffer_hdr_t *hdr = (odp_buffer_hdr_t *)ptr;
>  	size_t size = pool->s.hdr_size;
> -	uint8_t *payload = hdr->payload;
> +	uint8_t *payload = (uint8_t*)hdr + sizeof(odp_buffer_hdr_t);
>  
>  	if (buf_type == ODP_BUFFER_TYPE_CHUNK)
>  		size = sizeof(odp_buffer_chunk_hdr_t);
> -- 
> 1.8.5.1.163.gd7aced9
> 
> 
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
Maxim Uvarov June 5, 2014, 3:16 p.m. UTC | #2 
On 06/03/2014 11:41 AM, Anders Roxell wrote:
> On 2014-05-29 21:24, Maxim Uvarov wrote:
>> Compilation with c99 finds warning:
>>   field 'buf_hdr' with variable sized type
>>   'odp_buffer_hdr_t' (aka 'struct odp_buffer_hdr_t')
>>   not at the end of a struct or class is a GNU extension
>>
>>   This happens because odp_buffer_hdr_t struct has payload[]
>> at the and this struct is a part of other struct:
>>
>> typedef struct odp_buffer_chunk_hdr_t {
>> 	odp_buffer_hdr_t   buf_hdr; <- payload[] here
>> 	odp_buffer_chunk_t chunk;
>> } odp_buffer_chunk_hdr_t;
>>
>> This patch is propose to fix launchpad bug 1274577.
>>
>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
>> ---
>>   platform/linux-generic/include/odp_buffer_internal.h | 6 ++----
>>   platform/linux-generic/source/odp_buffer_pool.c      | 2 +-
>>   2 files changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/platform/linux-generic/include/odp_buffer_internal.h b/platform/linux-generic/include/odp_buffer_internal.h
>> index a1a6b4e..f2d409b 100644
>> --- a/platform/linux-generic/include/odp_buffer_internal.h
>> +++ b/platform/linux-generic/include/odp_buffer_internal.h
>> @@ -92,12 +92,10 @@ typedef struct odp_buffer_hdr_t {
>>   	int                      type;       /* type of next header */
>>   	odp_buffer_pool_t        pool;       /* buffer pool */
>>   
>> -	uint8_t                  payload[];  /* next header or data */
>> +	uint8_t 		 pad[32];    /* to pass check_align() */
> Why do we chose 32?
>
> - Anders
to pass check_align() bellow in the code as I wrote in the comment.


Maxim.

>
>> +	/* next header or data just after this sctuct */
>>   } odp_buffer_hdr_t;
>>   
>> -ODP_ASSERT(sizeof(odp_buffer_hdr_t) == ODP_OFFSETOF(odp_buffer_hdr_t, payload),
>> -	   ODP_BUFFER_HDR_T__SIZE_ERROR);
>> -
>>   
>>   typedef struct odp_buffer_chunk_hdr_t {
>>   	odp_buffer_hdr_t   buf_hdr;
>> diff --git a/platform/linux-generic/source/odp_buffer_pool.c b/platform/linux-generic/source/odp_buffer_pool.c
>> index 90214ba..0687bcb 100644
>> --- a/platform/linux-generic/source/odp_buffer_pool.c
>> +++ b/platform/linux-generic/source/odp_buffer_pool.c
>> @@ -206,7 +206,7 @@ static void fill_hdr(void *ptr, pool_entry_t *pool, uint32_t index,
>>   {
>>   	odp_buffer_hdr_t *hdr = (odp_buffer_hdr_t *)ptr;
>>   	size_t size = pool->s.hdr_size;
>> -	uint8_t *payload = hdr->payload;
>> +	uint8_t *payload = (uint8_t*)hdr + sizeof(odp_buffer_hdr_t);
>>   
>>   	if (buf_type == ODP_BUFFER_TYPE_CHUNK)
>>   		size = sizeof(odp_buffer_chunk_hdr_t);
>> -- 
>> 1.8.5.1.163.gd7aced9
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
Anders Roxell June 6, 2014, 9:33 a.m. UTC | #3 
On 2014-06-05 19:16, Maxim Uvarov wrote:
> On 06/03/2014 11:41 AM, Anders Roxell wrote:
> >On 2014-05-29 21:24, Maxim Uvarov wrote:
> >>Compilation with c99 finds warning:
> >>  field 'buf_hdr' with variable sized type
> >>  'odp_buffer_hdr_t' (aka 'struct odp_buffer_hdr_t')
> >>  not at the end of a struct or class is a GNU extension
> >>
> >>  This happens because odp_buffer_hdr_t struct has payload[]
> >>at the and this struct is a part of other struct:

When I read this again I don't understand this sentence.

> >>
> >>typedef struct odp_buffer_chunk_hdr_t {
> >>	odp_buffer_hdr_t   buf_hdr; <- payload[] here
> >>	odp_buffer_chunk_t chunk;
> >>} odp_buffer_chunk_hdr_t;
> >>
> >>This patch is propose to fix launchpad bug 1274577.
> >>
> >>Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> >>---
> >>  platform/linux-generic/include/odp_buffer_internal.h | 6 ++----
> >>  platform/linux-generic/source/odp_buffer_pool.c      | 2 +-
> >>  2 files changed, 3 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/platform/linux-generic/include/odp_buffer_internal.h b/platform/linux-generic/include/odp_buffer_internal.h
> >>index a1a6b4e..f2d409b 100644
> >>--- a/platform/linux-generic/include/odp_buffer_internal.h
> >>+++ b/platform/linux-generic/include/odp_buffer_internal.h
> >>@@ -92,12 +92,10 @@ typedef struct odp_buffer_hdr_t {
> >>  	int                      type;       /* type of next header */
> >>  	odp_buffer_pool_t        pool;       /* buffer pool */
> >>-	uint8_t                  payload[];  /* next header or data */
> >>+	uint8_t 		 pad[32];    /* to pass check_align() */
> >Why do we chose 32?
> >
> >- Anders
> to pass check_align() bellow in the code as I wrote in the comment.

I'm confused sorry.

The comments says we have to have the pad variable to pass
check_align() right?

And back to my original question:
Why did you choose that the array should be 32 and why not 64?


> 
> 
> Maxim.
> 
> >
> >>+	/* next header or data just after this sctuct */

nit: s/sctuct/struct/

Cheers,
Anders

> >>  } odp_buffer_hdr_t;
> >>-ODP_ASSERT(sizeof(odp_buffer_hdr_t) == ODP_OFFSETOF(odp_buffer_hdr_t, payload),
> >>-	   ODP_BUFFER_HDR_T__SIZE_ERROR);
> >>-
> >>  typedef struct odp_buffer_chunk_hdr_t {
> >>  	odp_buffer_hdr_t   buf_hdr;
> >>diff --git a/platform/linux-generic/source/odp_buffer_pool.c b/platform/linux-generic/source/odp_buffer_pool.c
> >>index 90214ba..0687bcb 100644
> >>--- a/platform/linux-generic/source/odp_buffer_pool.c
> >>+++ b/platform/linux-generic/source/odp_buffer_pool.c
> >>@@ -206,7 +206,7 @@ static void fill_hdr(void *ptr, pool_entry_t *pool, uint32_t index,
> >>  {
> >>  	odp_buffer_hdr_t *hdr = (odp_buffer_hdr_t *)ptr;
> >>  	size_t size = pool->s.hdr_size;
> >>-	uint8_t *payload = hdr->payload;
> >>+	uint8_t *payload = (uint8_t*)hdr + sizeof(odp_buffer_hdr_t);
> >>  	if (buf_type == ODP_BUFFER_TYPE_CHUNK)
> >>  		size = sizeof(odp_buffer_chunk_hdr_t);
> >>-- 
> >>1.8.5.1.163.gd7aced9
> >>
> >>
> >>_______________________________________________
> >>lng-odp mailing list
> >>lng-odp@lists.linaro.org
> >>http://lists.linaro.org/mailman/listinfo/lng-odp
>
Maxim Uvarov June 6, 2014, 9:45 a.m. UTC | #4 
On 06/06/2014 01:33 PM, Anders Roxell wrote:
> On 2014-06-05 19:16, Maxim Uvarov wrote:
>> On 06/03/2014 11:41 AM, Anders Roxell wrote:
>>> On 2014-05-29 21:24, Maxim Uvarov wrote:
>>>> Compilation with c99 finds warning:
>>>>   field 'buf_hdr' with variable sized type
>>>>   'odp_buffer_hdr_t' (aka 'struct odp_buffer_hdr_t')
>>>>   not at the end of a struct or class is a GNU extension
>>>>
>>>>   This happens because odp_buffer_hdr_t struct has payload[]
>>>> at the and this struct is a part of other struct:
> When I read this again I don't understand this sentence.
>
>>>> typedef struct odp_buffer_chunk_hdr_t {
>>>> 	odp_buffer_hdr_t   buf_hdr; <- payload[] here
>>>> 	odp_buffer_chunk_t chunk;
>>>> } odp_buffer_chunk_hdr_t;
>>>>
>>>> This patch is propose to fix launchpad bug 1274577.
>>>>
>>>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
>>>> ---
>>>>   platform/linux-generic/include/odp_buffer_internal.h | 6 ++----
>>>>   platform/linux-generic/source/odp_buffer_pool.c      | 2 +-
>>>>   2 files changed, 3 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/platform/linux-generic/include/odp_buffer_internal.h b/platform/linux-generic/include/odp_buffer_internal.h
>>>> index a1a6b4e..f2d409b 100644
>>>> --- a/platform/linux-generic/include/odp_buffer_internal.h
>>>> +++ b/platform/linux-generic/include/odp_buffer_internal.h
>>>> @@ -92,12 +92,10 @@ typedef struct odp_buffer_hdr_t {
>>>>   	int                      type;       /* type of next header */
>>>>   	odp_buffer_pool_t        pool;       /* buffer pool */
>>>> -	uint8_t                  payload[];  /* next header or data */
>>>> +	uint8_t 		 pad[32];    /* to pass check_align() */
>>> Why do we chose 32?
>>>
>>> - Anders
>> to pass check_align() bellow in the code as I wrote in the comment.
> I'm confused sorry.
>
> The comments says we have to have the pad variable to pass
> check_align() right?
in my opinion comment should mean "add unused padding to start packet 
data on required
address which is checked with check_align()

which is:

static void check_align(pool_entry_t *pool, odp_buffer_hdr_t *hdr)
{
     if (!ODP_ALIGNED_CHECK_POWER_2(hdr->addr, pool->s.payload_align)) {
         ODP_ERR("check_align: payload align error %p, align %zu\n",
             hdr->addr, pool->s.payload_align);
         exit(0);
     }

     if (!ODP_ALIGNED_CHECK_POWER_2(hdr, ODP_CACHE_LINE_SIZE)) {
         ODP_ERR("check_align: hdr align error %p, align %i\n",
             hdr, ODP_CACHE_LINE_SIZE);
         exit(0);
     }
}

>
> And back to my original question:
> Why did you choose that the array should be 32 and why not 64?
32 is minimal value to align structure.


Maxim.
>
>>
>> Maxim.
>>
>>>> +	/* next header or data just after this sctuct */
> nit: s/sctuct/struct/
>
> Cheers,
> Anders
>
>>>>   } odp_buffer_hdr_t;
>>>> -ODP_ASSERT(sizeof(odp_buffer_hdr_t) == ODP_OFFSETOF(odp_buffer_hdr_t, payload),
>>>> -	   ODP_BUFFER_HDR_T__SIZE_ERROR);
>>>> -
>>>>   typedef struct odp_buffer_chunk_hdr_t {
>>>>   	odp_buffer_hdr_t   buf_hdr;
>>>> diff --git a/platform/linux-generic/source/odp_buffer_pool.c b/platform/linux-generic/source/odp_buffer_pool.c
>>>> index 90214ba..0687bcb 100644
>>>> --- a/platform/linux-generic/source/odp_buffer_pool.c
>>>> +++ b/platform/linux-generic/source/odp_buffer_pool.c
>>>> @@ -206,7 +206,7 @@ static void fill_hdr(void *ptr, pool_entry_t *pool, uint32_t index,
>>>>   {
>>>>   	odp_buffer_hdr_t *hdr = (odp_buffer_hdr_t *)ptr;
>>>>   	size_t size = pool->s.hdr_size;
>>>> -	uint8_t *payload = hdr->payload;
>>>> +	uint8_t *payload = (uint8_t*)hdr + sizeof(odp_buffer_hdr_t);
>>>>   	if (buf_type == ODP_BUFFER_TYPE_CHUNK)
>>>>   		size = sizeof(odp_buffer_chunk_hdr_t);
>>>> -- 
>>>> 1.8.5.1.163.gd7aced9
>>>>
>>>>
>>>> _______________________________________________
>>>> lng-odp mailing list
>>>> lng-odp@lists.linaro.org
>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
diff mbox

Patch

diff --git a/platform/linux-generic/include/odp_buffer_internal.h b/platform/linux-generic/include/odp_buffer_internal.h
index a1a6b4e..f2d409b 100644
--- a/platform/linux-generic/include/odp_buffer_internal.h
+++ b/platform/linux-generic/include/odp_buffer_internal.h
@@ -92,12 +92,10 @@  typedef struct odp_buffer_hdr_t {
 	int                      type;       /* type of next header */
 	odp_buffer_pool_t        pool;       /* buffer pool */
 
-	uint8_t                  payload[];  /* next header or data */
+	uint8_t 		 pad[32];    /* to pass check_align() */
+	/* next header or data just after this sctuct */
 } odp_buffer_hdr_t;
 
-ODP_ASSERT(sizeof(odp_buffer_hdr_t) == ODP_OFFSETOF(odp_buffer_hdr_t, payload),
-	   ODP_BUFFER_HDR_T__SIZE_ERROR);
-
 
 typedef struct odp_buffer_chunk_hdr_t {
 	odp_buffer_hdr_t   buf_hdr;
diff --git a/platform/linux-generic/source/odp_buffer_pool.c b/platform/linux-generic/source/odp_buffer_pool.c
index 90214ba..0687bcb 100644
--- a/platform/linux-generic/source/odp_buffer_pool.c
+++ b/platform/linux-generic/source/odp_buffer_pool.c
@@ -206,7 +206,7 @@  static void fill_hdr(void *ptr, pool_entry_t *pool, uint32_t index,
 {
 	odp_buffer_hdr_t *hdr = (odp_buffer_hdr_t *)ptr;
 	size_t size = pool->s.hdr_size;
-	uint8_t *payload = hdr->payload;
+	uint8_t *payload = (uint8_t*)hdr + sizeof(odp_buffer_hdr_t);
 
 	if (buf_type == ODP_BUFFER_TYPE_CHUNK)
 		size = sizeof(odp_buffer_chunk_hdr_t);