[PATCHv4] linux-generic: debug: enable helper use from c++ programs

Message ID 1486075915-28921-1-git-send-email-bill.fischofer@linaro.org
State New
Headers show

Commit Message

Bill Fischofer Feb. 2, 2017, 10:51 p.m.
The ODP_STATIC_ASSERT() macro expands to _Static_assert(), however when
used in C++ programs this needs to expand to static_assert().

This resolves Bug https://bugs.linaro.org/show_bug.cgi?id=2852

Reported-by: Moshe Tubul <moshe.tubul@firmitas-cs.com>
Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>

---
Changes for v4:
- Remove extraneous debug code

Changes for v3:
- Support older versions of g++ that don't allow c++11 static assert by default

Changes for v2:
- Update C++ test case to include helper apis to validate this fix

 platform/linux-generic/include/odp/api/debug.h      | 21 +++++++++++++++------
 test/common_plat/miscellaneous/odp_api_from_cpp.cpp |  3 +--
 2 files changed, 16 insertions(+), 8 deletions(-)

-- 
2.7.4

Comments

Bill Fischofer Feb. 6, 2017, 7:14 p.m. | #1
Ping. Brian can you please verify that this now works on your system.
It works on my Ubuntu 16.04 system.  A review would also be nice. :)

On Thu, Feb 2, 2017 at 4:51 PM, Bill Fischofer
<bill.fischofer@linaro.org> wrote:
> The ODP_STATIC_ASSERT() macro expands to _Static_assert(), however when

> used in C++ programs this needs to expand to static_assert().

>

> This resolves Bug https://bugs.linaro.org/show_bug.cgi?id=2852

>

> Reported-by: Moshe Tubul <moshe.tubul@firmitas-cs.com>

> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>

> ---

> Changes for v4:

> - Remove extraneous debug code

>

> Changes for v3:

> - Support older versions of g++ that don't allow c++11 static assert by default

>

> Changes for v2:

> - Update C++ test case to include helper apis to validate this fix

>

>  platform/linux-generic/include/odp/api/debug.h      | 21 +++++++++++++++------

>  test/common_plat/miscellaneous/odp_api_from_cpp.cpp |  3 +--

>  2 files changed, 16 insertions(+), 8 deletions(-)

>

> diff --git a/platform/linux-generic/include/odp/api/debug.h b/platform/linux-generic/include/odp/api/debug.h

> index 7db1433..44ca5b7 100644

> --- a/platform/linux-generic/include/odp/api/debug.h

> +++ b/platform/linux-generic/include/odp/api/debug.h

> @@ -19,18 +19,25 @@ extern "C" {

>

>  #include <odp/api/spec/debug.h>

>

> -#if defined(__GNUC__) && !defined(__clang__)

> +#if defined(__GNUC__)

>

> -#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6))

> +#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6)) || \

> +       (__GNUC__ < 6 && defined(__cplusplus))

>

>  /**

> - * @internal _Static_assert was only added in GCC 4.6. Provide a weak replacement

> - * for previous versions.

> + * @internal _Static_assert was only added in GCC 4.6 and the C++ version

> + * static_assert for g++ 6 and above.  Provide a weak replacement for previous

> + * versions.

>   */

> +#ifdef __cplusplus

> +#define static_assert(e, s) \

> +       extern int _sassert[(e) ? 1 : -1]

> +#else

>  #define _Static_assert(e, s) (extern int (*static_assert_checker(void)) \

>         [sizeof(struct { unsigned int error_if_negative:(e) ? 1 : -1; })])

>

>  #endif

> +#endif

>

>  #endif

>

> @@ -39,9 +46,11 @@ extern "C" {

>   * if condition 'cond' is false. Macro definition is empty when compiler is not

>   * supported or the compiler does not support static assertion.

>   */

> -#define ODP_STATIC_ASSERT(cond, msg)  _Static_assert(cond, msg)

> +#ifndef __cplusplus

> +#define ODP_STATIC_ASSERT(cond, msg) _Static_assert(cond, msg)

>

> -#ifdef __cplusplus

> +#else

> +#define ODP_STATIC_ASSERT(cond, msg) static_assert(cond, msg)

>  }

>  #endif

>

> diff --git a/test/common_plat/miscellaneous/odp_api_from_cpp.cpp b/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

> index 2b30786..4578ae4 100644

> --- a/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

> +++ b/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

> @@ -1,10 +1,9 @@

>  #include <cstdio>

>  #include <odp_api.h>

> -#include <odp/helper/threads.h>

> +#include <odp/helper/odph_api.h>

>

>  int main(int argc ODP_UNUSED, const char *argv[] ODP_UNUSED)

>  {

> -

>         printf("\tODP API version: %s\n", odp_version_api_str());

>         printf("\tODP implementation version: %s\n", odp_version_impl_str());

>

> --

> 2.7.4

>
Brian Brooks Feb. 7, 2017, 2:51 a.m. | #2
On 02/06 13:14:17, Bill Fischofer wrote:
> Ping. Brian can you please verify that this now works on your system.

> It works on my Ubuntu 16.04 system.


Just built on:

Ubuntu 16.10 - x86_64
Ubuntu 16.04 - aarch64
Arch         - aarch64 & x86_64

so the previous build error I reported seems to have been resolved.

> A review would also be nice. :)


LGTM, please...

Reviewed-and-tested-by: Brian Brooks <brian.brooks@linaro.org>

> On Thu, Feb 2, 2017 at 4:51 PM, Bill Fischofer

> <bill.fischofer@linaro.org> wrote:

> > The ODP_STATIC_ASSERT() macro expands to _Static_assert(), however when

> > used in C++ programs this needs to expand to static_assert().

> >

> > This resolves Bug https://bugs.linaro.org/show_bug.cgi?id=2852

> >

> > Reported-by: Moshe Tubul <moshe.tubul@firmitas-cs.com>

> > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>

> > ---

> > Changes for v4:

> > - Remove extraneous debug code

> >

> > Changes for v3:

> > - Support older versions of g++ that don't allow c++11 static assert by default

> >

> > Changes for v2:

> > - Update C++ test case to include helper apis to validate this fix

> >

> >  platform/linux-generic/include/odp/api/debug.h      | 21 +++++++++++++++------

> >  test/common_plat/miscellaneous/odp_api_from_cpp.cpp |  3 +--

> >  2 files changed, 16 insertions(+), 8 deletions(-)

> >

> > diff --git a/platform/linux-generic/include/odp/api/debug.h b/platform/linux-generic/include/odp/api/debug.h

> > index 7db1433..44ca5b7 100644

> > --- a/platform/linux-generic/include/odp/api/debug.h

> > +++ b/platform/linux-generic/include/odp/api/debug.h

> > @@ -19,18 +19,25 @@ extern "C" {

> >

> >  #include <odp/api/spec/debug.h>

> >

> > -#if defined(__GNUC__) && !defined(__clang__)

> > +#if defined(__GNUC__)

> >

> > -#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6))

> > +#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6)) || \

> > +       (__GNUC__ < 6 && defined(__cplusplus))

> >

> >  /**

> > - * @internal _Static_assert was only added in GCC 4.6. Provide a weak replacement

> > - * for previous versions.

> > + * @internal _Static_assert was only added in GCC 4.6 and the C++ version

> > + * static_assert for g++ 6 and above.  Provide a weak replacement for previous

> > + * versions.

> >   */


I think a plain C-style build time assertion (no special compiler support
needed - besides unused attribute) will work for C++ as well. So the entire
thing could just be:

  #define _SA1(cond_, line_) \
    extern int __static_assertion_ ## line_[1 - 2*!(cond_)] __unused
  #define _SA0(cond_, line_) _SA1(cond_, line_)
  #define ODP_STATIC_ASSERT(cond) _SA0(cond, __LINE__)

It also should be deprecated from the layer in which it currently resides
and used internally only; it adds no value at the data plane abstraction
layer. Code bases will already be using their own mechanism for build time
assertions.

> > +#ifdef __cplusplus


Consistent use of "#if defined(identifier)" advised.

> > +#define static_assert(e, s) \

> > +       extern int _sassert[(e) ? 1 : -1]

> > +#else

> >  #define _Static_assert(e, s) (extern int (*static_assert_checker(void)) \

> >         [sizeof(struct { unsigned int error_if_negative:(e) ? 1 : -1; })])

> >

> >  #endif

> > +#endif

> >

> >  #endif

> >

> > @@ -39,9 +46,11 @@ extern "C" {

> >   * if condition 'cond' is false. Macro definition is empty when compiler is not

> >   * supported or the compiler does not support static assertion.

> >   */

> > -#define ODP_STATIC_ASSERT(cond, msg)  _Static_assert(cond, msg)

> > +#ifndef __cplusplus

> > +#define ODP_STATIC_ASSERT(cond, msg) _Static_assert(cond, msg)

> >


Newline here could be removed.

> > -#ifdef __cplusplus

> > +#else

> > +#define ODP_STATIC_ASSERT(cond, msg) static_assert(cond, msg)

> >  }

> >  #endif

> >

> > diff --git a/test/common_plat/miscellaneous/odp_api_from_cpp.cpp b/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

> > index 2b30786..4578ae4 100644

> > --- a/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

> > +++ b/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

> > @@ -1,10 +1,9 @@

> >  #include <cstdio>

> >  #include <odp_api.h>

> > -#include <odp/helper/threads.h>

> > +#include <odp/helper/odph_api.h>

> >

> >  int main(int argc ODP_UNUSED, const char *argv[] ODP_UNUSED)

> >  {

> > -

> >         printf("\tODP API version: %s\n", odp_version_api_str());

> >         printf("\tODP implementation version: %s\n", odp_version_impl_str());

> >

> > --

> > 2.7.4

> >
Maxim Uvarov Feb. 7, 2017, 1:30 p.m. | #3
I pushed it to github and test failed:

https://api.travis-ci.org/jobs/199201799/log.txt?deansi=true

Maxim.


On 02/07/17 05:51, Brian Brooks wrote:
> On 02/06 13:14:17, Bill Fischofer wrote:

>> Ping. Brian can you please verify that this now works on your system.

>> It works on my Ubuntu 16.04 system.

> 

> Just built on:

> 

> Ubuntu 16.10 - x86_64

> Ubuntu 16.04 - aarch64

> Arch         - aarch64 & x86_64

> 

> so the previous build error I reported seems to have been resolved.

> 

>> A review would also be nice. :)

> 

> LGTM, please...

> 

> Reviewed-and-tested-by: Brian Brooks <brian.brooks@linaro.org>

> 

>> On Thu, Feb 2, 2017 at 4:51 PM, Bill Fischofer

>> <bill.fischofer@linaro.org> wrote:

>>> The ODP_STATIC_ASSERT() macro expands to _Static_assert(), however when

>>> used in C++ programs this needs to expand to static_assert().

>>>

>>> This resolves Bug https://bugs.linaro.org/show_bug.cgi?id=2852

>>>

>>> Reported-by: Moshe Tubul <moshe.tubul@firmitas-cs.com>

>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>

>>> ---

>>> Changes for v4:

>>> - Remove extraneous debug code

>>>

>>> Changes for v3:

>>> - Support older versions of g++ that don't allow c++11 static assert by default

>>>

>>> Changes for v2:

>>> - Update C++ test case to include helper apis to validate this fix

>>>

>>>  platform/linux-generic/include/odp/api/debug.h      | 21 +++++++++++++++------

>>>  test/common_plat/miscellaneous/odp_api_from_cpp.cpp |  3 +--

>>>  2 files changed, 16 insertions(+), 8 deletions(-)

>>>

>>> diff --git a/platform/linux-generic/include/odp/api/debug.h b/platform/linux-generic/include/odp/api/debug.h

>>> index 7db1433..44ca5b7 100644

>>> --- a/platform/linux-generic/include/odp/api/debug.h

>>> +++ b/platform/linux-generic/include/odp/api/debug.h

>>> @@ -19,18 +19,25 @@ extern "C" {

>>>

>>>  #include <odp/api/spec/debug.h>

>>>

>>> -#if defined(__GNUC__) && !defined(__clang__)

>>> +#if defined(__GNUC__)

>>>

>>> -#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6))

>>> +#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6)) || \

>>> +       (__GNUC__ < 6 && defined(__cplusplus))

>>>

>>>  /**

>>> - * @internal _Static_assert was only added in GCC 4.6. Provide a weak replacement

>>> - * for previous versions.

>>> + * @internal _Static_assert was only added in GCC 4.6 and the C++ version

>>> + * static_assert for g++ 6 and above.  Provide a weak replacement for previous

>>> + * versions.

>>>   */

> 

> I think a plain C-style build time assertion (no special compiler support

> needed - besides unused attribute) will work for C++ as well. So the entire

> thing could just be:

> 

>   #define _SA1(cond_, line_) \

>     extern int __static_assertion_ ## line_[1 - 2*!(cond_)] __unused

>   #define _SA0(cond_, line_) _SA1(cond_, line_)

>   #define ODP_STATIC_ASSERT(cond) _SA0(cond, __LINE__)

> 

> It also should be deprecated from the layer in which it currently resides

> and used internally only; it adds no value at the data plane abstraction

> layer. Code bases will already be using their own mechanism for build time

> assertions.

> 

>>> +#ifdef __cplusplus

> 

> Consistent use of "#if defined(identifier)" advised.

> 

>>> +#define static_assert(e, s) \

>>> +       extern int _sassert[(e) ? 1 : -1]

>>> +#else

>>>  #define _Static_assert(e, s) (extern int (*static_assert_checker(void)) \

>>>         [sizeof(struct { unsigned int error_if_negative:(e) ? 1 : -1; })])

>>>

>>>  #endif

>>> +#endif

>>>

>>>  #endif

>>>

>>> @@ -39,9 +46,11 @@ extern "C" {

>>>   * if condition 'cond' is false. Macro definition is empty when compiler is not

>>>   * supported or the compiler does not support static assertion.

>>>   */

>>> -#define ODP_STATIC_ASSERT(cond, msg)  _Static_assert(cond, msg)

>>> +#ifndef __cplusplus

>>> +#define ODP_STATIC_ASSERT(cond, msg) _Static_assert(cond, msg)

>>>

> 

> Newline here could be removed.

> 

>>> -#ifdef __cplusplus

>>> +#else

>>> +#define ODP_STATIC_ASSERT(cond, msg) static_assert(cond, msg)

>>>  }

>>>  #endif

>>>

>>> diff --git a/test/common_plat/miscellaneous/odp_api_from_cpp.cpp b/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>> index 2b30786..4578ae4 100644

>>> --- a/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>> +++ b/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>> @@ -1,10 +1,9 @@

>>>  #include <cstdio>

>>>  #include <odp_api.h>

>>> -#include <odp/helper/threads.h>

>>> +#include <odp/helper/odph_api.h>

>>>

>>>  int main(int argc ODP_UNUSED, const char *argv[] ODP_UNUSED)

>>>  {

>>> -

>>>         printf("\tODP API version: %s\n", odp_version_api_str());

>>>         printf("\tODP implementation version: %s\n", odp_version_impl_str());

>>>

>>> --

>>> 2.7.4

>>>
Bill Fischofer Feb. 7, 2017, 1:39 p.m. | #4
The errors being reported here are on unchanged code paths with an
older clang version (3.4.0 vs.4.2.1). Not sure why you'd see that here
and not in the unchanged code.

Also, this run is reporting doxygen issues with the traffic manager
that I don't see running locally:

$ make doxygen-doc
  DXGEN  doc/application-api-guide/Doxyfile
warning: ignoring unsupported tag `HTML_EXTRA_STYLESHEET =' at line
26, file ./doc/Doxyfile_common
error: Unexpected start tag `services' found in scope='class/memberdecl/'!
error: Unexpected start tag `interfaces' found in scope='class/memberdecl/'!
error: Unexpected start tag `services' found in scope='class/memberdef/'!
error: Unexpected start tag `interfaces' found in scope='class/memberdef/'!
error: Unexpected start tag `constantgroups' found in
scope='namespace/memberdecl/'!
error: Unexpected start tag `constantgroups' found in scope='file/memberdecl/'!
/home/travis/build/muvarov/odp/doc/application-api-guide/api_guide_lines.dox:10:
warning: Found unknown command `\tableofcontents'
/home/travis/build/muvarov/odp/doc/application-api-guide/release.dox:8:
warning: Found unknown command `\tableofcontents'
/home/travis/build/muvarov/odp/include/odp/api/spec/traffic_mngr.h:1717:
warning: expected whitespace after param command
/home/travis/build/muvarov/odp/include/odp/api/spec/traffic_mngr.h:1691:
warning: argument 'inout' of command @param is not found in the
argument list of odp_tm_node_fanin_info(odp_tm_node_t tm_node,
odp_tm_node_fanin_info_t *info)
/home/travis/build/muvarov/odp/include/odp/api/spec/traffic_mngr.h:1691:
warning: The following parameters of
odp_tm_node_fanin_info(odp_tm_node_t tm_node, odp_tm_node_fanin_info_t
*info) are not documented:
  parameter 'info'
  DXGEN  doc/helper-guide/Doxyfile
warning: ignoring unsupported tag `HTML_EXTRA_STYLESHEET =' at line
31, file ./doc/helper-guide/Doxyfile
error: Unexpected start tag `services' found in scope='class/memberdecl/'!
error: Unexpected start tag `interfaces' found in scope='class/memberdecl/'!
error: Unexpected start tag `services' found in scope='class/memberdef/'!
error: Unexpected start tag `interfaces' found in scope='class/memberdef/'!
error: Unexpected start tag `constantgroups' found in
scope='namespace/memberdecl/'!
error: Unexpected start tag `constantgroups' found in scope='file/memberdecl/'!

So do we also have a doxygen version issue on these systems?

On Tue, Feb 7, 2017 at 7:30 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> I pushed it to github and test failed:

>

> https://api.travis-ci.org/jobs/199201799/log.txt?deansi=true

>

> Maxim.

>

>

> On 02/07/17 05:51, Brian Brooks wrote:

>> On 02/06 13:14:17, Bill Fischofer wrote:

>>> Ping. Brian can you please verify that this now works on your system.

>>> It works on my Ubuntu 16.04 system.

>>

>> Just built on:

>>

>> Ubuntu 16.10 - x86_64

>> Ubuntu 16.04 - aarch64

>> Arch         - aarch64 & x86_64

>>

>> so the previous build error I reported seems to have been resolved.

>>

>>> A review would also be nice. :)

>>

>> LGTM, please...

>>

>> Reviewed-and-tested-by: Brian Brooks <brian.brooks@linaro.org>

>>

>>> On Thu, Feb 2, 2017 at 4:51 PM, Bill Fischofer

>>> <bill.fischofer@linaro.org> wrote:

>>>> The ODP_STATIC_ASSERT() macro expands to _Static_assert(), however when

>>>> used in C++ programs this needs to expand to static_assert().

>>>>

>>>> This resolves Bug https://bugs.linaro.org/show_bug.cgi?id=2852

>>>>

>>>> Reported-by: Moshe Tubul <moshe.tubul@firmitas-cs.com>

>>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>

>>>> ---

>>>> Changes for v4:

>>>> - Remove extraneous debug code

>>>>

>>>> Changes for v3:

>>>> - Support older versions of g++ that don't allow c++11 static assert by default

>>>>

>>>> Changes for v2:

>>>> - Update C++ test case to include helper apis to validate this fix

>>>>

>>>>  platform/linux-generic/include/odp/api/debug.h      | 21 +++++++++++++++------

>>>>  test/common_plat/miscellaneous/odp_api_from_cpp.cpp |  3 +--

>>>>  2 files changed, 16 insertions(+), 8 deletions(-)

>>>>

>>>> diff --git a/platform/linux-generic/include/odp/api/debug.h b/platform/linux-generic/include/odp/api/debug.h

>>>> index 7db1433..44ca5b7 100644

>>>> --- a/platform/linux-generic/include/odp/api/debug.h

>>>> +++ b/platform/linux-generic/include/odp/api/debug.h

>>>> @@ -19,18 +19,25 @@ extern "C" {

>>>>

>>>>  #include <odp/api/spec/debug.h>

>>>>

>>>> -#if defined(__GNUC__) && !defined(__clang__)

>>>> +#if defined(__GNUC__)

>>>>

>>>> -#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6))

>>>> +#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6)) || \

>>>> +       (__GNUC__ < 6 && defined(__cplusplus))

>>>>

>>>>  /**

>>>> - * @internal _Static_assert was only added in GCC 4.6. Provide a weak replacement

>>>> - * for previous versions.

>>>> + * @internal _Static_assert was only added in GCC 4.6 and the C++ version

>>>> + * static_assert for g++ 6 and above.  Provide a weak replacement for previous

>>>> + * versions.

>>>>   */

>>

>> I think a plain C-style build time assertion (no special compiler support

>> needed - besides unused attribute) will work for C++ as well. So the entire

>> thing could just be:

>>

>>   #define _SA1(cond_, line_) \

>>     extern int __static_assertion_ ## line_[1 - 2*!(cond_)] __unused

>>   #define _SA0(cond_, line_) _SA1(cond_, line_)

>>   #define ODP_STATIC_ASSERT(cond) _SA0(cond, __LINE__)

>>

>> It also should be deprecated from the layer in which it currently resides

>> and used internally only; it adds no value at the data plane abstraction

>> layer. Code bases will already be using their own mechanism for build time

>> assertions.

>>

>>>> +#ifdef __cplusplus

>>

>> Consistent use of "#if defined(identifier)" advised.

>>

>>>> +#define static_assert(e, s) \

>>>> +       extern int _sassert[(e) ? 1 : -1]

>>>> +#else

>>>>  #define _Static_assert(e, s) (extern int (*static_assert_checker(void)) \

>>>>         [sizeof(struct { unsigned int error_if_negative:(e) ? 1 : -1; })])

>>>>

>>>>  #endif

>>>> +#endif

>>>>

>>>>  #endif

>>>>

>>>> @@ -39,9 +46,11 @@ extern "C" {

>>>>   * if condition 'cond' is false. Macro definition is empty when compiler is not

>>>>   * supported or the compiler does not support static assertion.

>>>>   */

>>>> -#define ODP_STATIC_ASSERT(cond, msg)  _Static_assert(cond, msg)

>>>> +#ifndef __cplusplus

>>>> +#define ODP_STATIC_ASSERT(cond, msg) _Static_assert(cond, msg)

>>>>

>>

>> Newline here could be removed.

>>

>>>> -#ifdef __cplusplus

>>>> +#else

>>>> +#define ODP_STATIC_ASSERT(cond, msg) static_assert(cond, msg)

>>>>  }

>>>>  #endif

>>>>

>>>> diff --git a/test/common_plat/miscellaneous/odp_api_from_cpp.cpp b/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>>> index 2b30786..4578ae4 100644

>>>> --- a/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>>> +++ b/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>>> @@ -1,10 +1,9 @@

>>>>  #include <cstdio>

>>>>  #include <odp_api.h>

>>>> -#include <odp/helper/threads.h>

>>>> +#include <odp/helper/odph_api.h>

>>>>

>>>>  int main(int argc ODP_UNUSED, const char *argv[] ODP_UNUSED)

>>>>  {

>>>> -

>>>>         printf("\tODP API version: %s\n", odp_version_api_str());

>>>>         printf("\tODP implementation version: %s\n", odp_version_impl_str());

>>>>

>>>> --

>>>> 2.7.4

>>>>

>
Maxim Uvarov Feb. 7, 2017, 1:51 p.m. | #5
On 02/07/17 16:39, Bill Fischofer wrote:
> The errors being reported here are on unchanged code paths with an

> older clang version (3.4.0 vs.4.2.1). Not sure why you'd see that here

> and not in the unchanged code.

> 

> Also, this run is reporting doxygen issues with the traffic manager

> that I don't see running locally:

> 

> $ make doxygen-doc

>   DXGEN  doc/application-api-guide/Doxyfile

> warning: ignoring unsupported tag `HTML_EXTRA_STYLESHEET =' at line

> 26, file ./doc/Doxyfile_common

> error: Unexpected start tag `services' found in scope='class/memberdecl/'!

> error: Unexpected start tag `interfaces' found in scope='class/memberdecl/'!

> error: Unexpected start tag `services' found in scope='class/memberdef/'!

> error: Unexpected start tag `interfaces' found in scope='class/memberdef/'!

> error: Unexpected start tag `constantgroups' found in

> scope='namespace/memberdecl/'!

> error: Unexpected start tag `constantgroups' found in scope='file/memberdecl/'!

> /home/travis/build/muvarov/odp/doc/application-api-guide/api_guide_lines.dox:10:

> warning: Found unknown command `\tableofcontents'

> /home/travis/build/muvarov/odp/doc/application-api-guide/release.dox:8:

> warning: Found unknown command `\tableofcontents'

> /home/travis/build/muvarov/odp/include/odp/api/spec/traffic_mngr.h:1717:

> warning: expected whitespace after param command

> /home/travis/build/muvarov/odp/include/odp/api/spec/traffic_mngr.h:1691:

> warning: argument 'inout' of command @param is not found in the

> argument list of odp_tm_node_fanin_info(odp_tm_node_t tm_node,

> odp_tm_node_fanin_info_t *info)

> /home/travis/build/muvarov/odp/include/odp/api/spec/traffic_mngr.h:1691:

> warning: The following parameters of

> odp_tm_node_fanin_info(odp_tm_node_t tm_node, odp_tm_node_fanin_info_t

> *info) are not documented:

>   parameter 'info'

>   DXGEN  doc/helper-guide/Doxyfile

> warning: ignoring unsupported tag `HTML_EXTRA_STYLESHEET =' at line

> 31, file ./doc/helper-guide/Doxyfile

> error: Unexpected start tag `services' found in scope='class/memberdecl/'!

> error: Unexpected start tag `interfaces' found in scope='class/memberdecl/'!

> error: Unexpected start tag `services' found in scope='class/memberdef/'!

> error: Unexpected start tag `interfaces' found in scope='class/memberdef/'!

> error: Unexpected start tag `constantgroups' found in

> scope='namespace/memberdecl/'!

> error: Unexpected start tag `constantgroups' found in scope='file/memberdecl/'!

> 

> So do we also have a doxygen version issue on these systems?

> 



looks like yes. I also do not see comment locally.

we also need some way to return non 0 if doxygen reported an error.

The command "make doxygen-doc" exited with 0.



> On Tue, Feb 7, 2017 at 7:30 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

>> I pushed it to github and test failed:

>>

>> https://api.travis-ci.org/jobs/199201799/log.txt?deansi=true

>>

>> Maxim.

>>

>>

>> On 02/07/17 05:51, Brian Brooks wrote:

>>> On 02/06 13:14:17, Bill Fischofer wrote:

>>>> Ping. Brian can you please verify that this now works on your system.

>>>> It works on my Ubuntu 16.04 system.

>>>

>>> Just built on:

>>>

>>> Ubuntu 16.10 - x86_64

>>> Ubuntu 16.04 - aarch64

>>> Arch         - aarch64 & x86_64

>>>

>>> so the previous build error I reported seems to have been resolved.

>>>

>>>> A review would also be nice. :)

>>>

>>> LGTM, please...

>>>

>>> Reviewed-and-tested-by: Brian Brooks <brian.brooks@linaro.org>

>>>

>>>> On Thu, Feb 2, 2017 at 4:51 PM, Bill Fischofer

>>>> <bill.fischofer@linaro.org> wrote:

>>>>> The ODP_STATIC_ASSERT() macro expands to _Static_assert(), however when

>>>>> used in C++ programs this needs to expand to static_assert().

>>>>>

>>>>> This resolves Bug https://bugs.linaro.org/show_bug.cgi?id=2852

>>>>>

>>>>> Reported-by: Moshe Tubul <moshe.tubul@firmitas-cs.com>

>>>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>

>>>>> ---

>>>>> Changes for v4:

>>>>> - Remove extraneous debug code

>>>>>

>>>>> Changes for v3:

>>>>> - Support older versions of g++ that don't allow c++11 static assert by default

>>>>>

>>>>> Changes for v2:

>>>>> - Update C++ test case to include helper apis to validate this fix

>>>>>

>>>>>  platform/linux-generic/include/odp/api/debug.h      | 21 +++++++++++++++------

>>>>>  test/common_plat/miscellaneous/odp_api_from_cpp.cpp |  3 +--

>>>>>  2 files changed, 16 insertions(+), 8 deletions(-)

>>>>>

>>>>> diff --git a/platform/linux-generic/include/odp/api/debug.h b/platform/linux-generic/include/odp/api/debug.h

>>>>> index 7db1433..44ca5b7 100644

>>>>> --- a/platform/linux-generic/include/odp/api/debug.h

>>>>> +++ b/platform/linux-generic/include/odp/api/debug.h

>>>>> @@ -19,18 +19,25 @@ extern "C" {

>>>>>

>>>>>  #include <odp/api/spec/debug.h>

>>>>>

>>>>> -#if defined(__GNUC__) && !defined(__clang__)

>>>>> +#if defined(__GNUC__)

>>>>>

>>>>> -#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6))

>>>>> +#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6)) || \

>>>>> +       (__GNUC__ < 6 && defined(__cplusplus))

>>>>>

>>>>>  /**

>>>>> - * @internal _Static_assert was only added in GCC 4.6. Provide a weak replacement

>>>>> - * for previous versions.

>>>>> + * @internal _Static_assert was only added in GCC 4.6 and the C++ version

>>>>> + * static_assert for g++ 6 and above.  Provide a weak replacement for previous

>>>>> + * versions.

>>>>>   */

>>>

>>> I think a plain C-style build time assertion (no special compiler support

>>> needed - besides unused attribute) will work for C++ as well. So the entire

>>> thing could just be:

>>>

>>>   #define _SA1(cond_, line_) \

>>>     extern int __static_assertion_ ## line_[1 - 2*!(cond_)] __unused

>>>   #define _SA0(cond_, line_) _SA1(cond_, line_)

>>>   #define ODP_STATIC_ASSERT(cond) _SA0(cond, __LINE__)

>>>

>>> It also should be deprecated from the layer in which it currently resides

>>> and used internally only; it adds no value at the data plane abstraction

>>> layer. Code bases will already be using their own mechanism for build time

>>> assertions.

>>>

>>>>> +#ifdef __cplusplus

>>>

>>> Consistent use of "#if defined(identifier)" advised.

>>>

>>>>> +#define static_assert(e, s) \

>>>>> +       extern int _sassert[(e) ? 1 : -1]

>>>>> +#else

>>>>>  #define _Static_assert(e, s) (extern int (*static_assert_checker(void)) \

>>>>>         [sizeof(struct { unsigned int error_if_negative:(e) ? 1 : -1; })])

>>>>>

>>>>>  #endif

>>>>> +#endif

>>>>>

>>>>>  #endif

>>>>>

>>>>> @@ -39,9 +46,11 @@ extern "C" {

>>>>>   * if condition 'cond' is false. Macro definition is empty when compiler is not

>>>>>   * supported or the compiler does not support static assertion.

>>>>>   */

>>>>> -#define ODP_STATIC_ASSERT(cond, msg)  _Static_assert(cond, msg)

>>>>> +#ifndef __cplusplus

>>>>> +#define ODP_STATIC_ASSERT(cond, msg) _Static_assert(cond, msg)

>>>>>

>>>

>>> Newline here could be removed.

>>>

>>>>> -#ifdef __cplusplus

>>>>> +#else

>>>>> +#define ODP_STATIC_ASSERT(cond, msg) static_assert(cond, msg)

>>>>>  }

>>>>>  #endif

>>>>>

>>>>> diff --git a/test/common_plat/miscellaneous/odp_api_from_cpp.cpp b/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>>>> index 2b30786..4578ae4 100644

>>>>> --- a/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>>>> +++ b/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>>>> @@ -1,10 +1,9 @@

>>>>>  #include <cstdio>

>>>>>  #include <odp_api.h>

>>>>> -#include <odp/helper/threads.h>

>>>>> +#include <odp/helper/odph_api.h>

>>>>>

>>>>>  int main(int argc ODP_UNUSED, const char *argv[] ODP_UNUSED)

>>>>>  {

>>>>> -

>>>>>         printf("\tODP API version: %s\n", odp_version_api_str());

>>>>>         printf("\tODP implementation version: %s\n", odp_version_impl_str());

>>>>>

>>>>> --

>>>>> 2.7.4

>>>>>

>>
Bill Fischofer Feb. 7, 2017, 2:42 p.m. | #6
Given these issues with obsolete compiler levels, it may be simpler to
go with Brian's suggestion and just use some simple macros. I'll post
a v4 along those lines.

On Tue, Feb 7, 2017 at 7:51 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> On 02/07/17 16:39, Bill Fischofer wrote:

>> The errors being reported here are on unchanged code paths with an

>> older clang version (3.4.0 vs.4.2.1). Not sure why you'd see that here

>> and not in the unchanged code.

>>

>> Also, this run is reporting doxygen issues with the traffic manager

>> that I don't see running locally:

>>

>> $ make doxygen-doc

>>   DXGEN  doc/application-api-guide/Doxyfile

>> warning: ignoring unsupported tag `HTML_EXTRA_STYLESHEET =' at line

>> 26, file ./doc/Doxyfile_common

>> error: Unexpected start tag `services' found in scope='class/memberdecl/'!

>> error: Unexpected start tag `interfaces' found in scope='class/memberdecl/'!

>> error: Unexpected start tag `services' found in scope='class/memberdef/'!

>> error: Unexpected start tag `interfaces' found in scope='class/memberdef/'!

>> error: Unexpected start tag `constantgroups' found in

>> scope='namespace/memberdecl/'!

>> error: Unexpected start tag `constantgroups' found in scope='file/memberdecl/'!

>> /home/travis/build/muvarov/odp/doc/application-api-guide/api_guide_lines.dox:10:

>> warning: Found unknown command `\tableofcontents'

>> /home/travis/build/muvarov/odp/doc/application-api-guide/release.dox:8:

>> warning: Found unknown command `\tableofcontents'

>> /home/travis/build/muvarov/odp/include/odp/api/spec/traffic_mngr.h:1717:

>> warning: expected whitespace after param command

>> /home/travis/build/muvarov/odp/include/odp/api/spec/traffic_mngr.h:1691:

>> warning: argument 'inout' of command @param is not found in the

>> argument list of odp_tm_node_fanin_info(odp_tm_node_t tm_node,

>> odp_tm_node_fanin_info_t *info)

>> /home/travis/build/muvarov/odp/include/odp/api/spec/traffic_mngr.h:1691:

>> warning: The following parameters of

>> odp_tm_node_fanin_info(odp_tm_node_t tm_node, odp_tm_node_fanin_info_t

>> *info) are not documented:

>>   parameter 'info'

>>   DXGEN  doc/helper-guide/Doxyfile

>> warning: ignoring unsupported tag `HTML_EXTRA_STYLESHEET =' at line

>> 31, file ./doc/helper-guide/Doxyfile

>> error: Unexpected start tag `services' found in scope='class/memberdecl/'!

>> error: Unexpected start tag `interfaces' found in scope='class/memberdecl/'!

>> error: Unexpected start tag `services' found in scope='class/memberdef/'!

>> error: Unexpected start tag `interfaces' found in scope='class/memberdef/'!

>> error: Unexpected start tag `constantgroups' found in

>> scope='namespace/memberdecl/'!

>> error: Unexpected start tag `constantgroups' found in scope='file/memberdecl/'!

>>

>> So do we also have a doxygen version issue on these systems?

>>

>

>

> looks like yes. I also do not see comment locally.

>

> we also need some way to return non 0 if doxygen reported an error.

>

> The command "make doxygen-doc" exited with 0.

>

>

>

>> On Tue, Feb 7, 2017 at 7:30 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

>>> I pushed it to github and test failed:

>>>

>>> https://api.travis-ci.org/jobs/199201799/log.txt?deansi=true

>>>

>>> Maxim.

>>>

>>>

>>> On 02/07/17 05:51, Brian Brooks wrote:

>>>> On 02/06 13:14:17, Bill Fischofer wrote:

>>>>> Ping. Brian can you please verify that this now works on your system.

>>>>> It works on my Ubuntu 16.04 system.

>>>>

>>>> Just built on:

>>>>

>>>> Ubuntu 16.10 - x86_64

>>>> Ubuntu 16.04 - aarch64

>>>> Arch         - aarch64 & x86_64

>>>>

>>>> so the previous build error I reported seems to have been resolved.

>>>>

>>>>> A review would also be nice. :)

>>>>

>>>> LGTM, please...

>>>>

>>>> Reviewed-and-tested-by: Brian Brooks <brian.brooks@linaro.org>

>>>>

>>>>> On Thu, Feb 2, 2017 at 4:51 PM, Bill Fischofer

>>>>> <bill.fischofer@linaro.org> wrote:

>>>>>> The ODP_STATIC_ASSERT() macro expands to _Static_assert(), however when

>>>>>> used in C++ programs this needs to expand to static_assert().

>>>>>>

>>>>>> This resolves Bug https://bugs.linaro.org/show_bug.cgi?id=2852

>>>>>>

>>>>>> Reported-by: Moshe Tubul <moshe.tubul@firmitas-cs.com>

>>>>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>

>>>>>> ---

>>>>>> Changes for v4:

>>>>>> - Remove extraneous debug code

>>>>>>

>>>>>> Changes for v3:

>>>>>> - Support older versions of g++ that don't allow c++11 static assert by default

>>>>>>

>>>>>> Changes for v2:

>>>>>> - Update C++ test case to include helper apis to validate this fix

>>>>>>

>>>>>>  platform/linux-generic/include/odp/api/debug.h      | 21 +++++++++++++++------

>>>>>>  test/common_plat/miscellaneous/odp_api_from_cpp.cpp |  3 +--

>>>>>>  2 files changed, 16 insertions(+), 8 deletions(-)

>>>>>>

>>>>>> diff --git a/platform/linux-generic/include/odp/api/debug.h b/platform/linux-generic/include/odp/api/debug.h

>>>>>> index 7db1433..44ca5b7 100644

>>>>>> --- a/platform/linux-generic/include/odp/api/debug.h

>>>>>> +++ b/platform/linux-generic/include/odp/api/debug.h

>>>>>> @@ -19,18 +19,25 @@ extern "C" {

>>>>>>

>>>>>>  #include <odp/api/spec/debug.h>

>>>>>>

>>>>>> -#if defined(__GNUC__) && !defined(__clang__)

>>>>>> +#if defined(__GNUC__)

>>>>>>

>>>>>> -#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6))

>>>>>> +#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6)) || \

>>>>>> +       (__GNUC__ < 6 && defined(__cplusplus))

>>>>>>

>>>>>>  /**

>>>>>> - * @internal _Static_assert was only added in GCC 4.6. Provide a weak replacement

>>>>>> - * for previous versions.

>>>>>> + * @internal _Static_assert was only added in GCC 4.6 and the C++ version

>>>>>> + * static_assert for g++ 6 and above.  Provide a weak replacement for previous

>>>>>> + * versions.

>>>>>>   */

>>>>

>>>> I think a plain C-style build time assertion (no special compiler support

>>>> needed - besides unused attribute) will work for C++ as well. So the entire

>>>> thing could just be:

>>>>

>>>>   #define _SA1(cond_, line_) \

>>>>     extern int __static_assertion_ ## line_[1 - 2*!(cond_)] __unused

>>>>   #define _SA0(cond_, line_) _SA1(cond_, line_)

>>>>   #define ODP_STATIC_ASSERT(cond) _SA0(cond, __LINE__)

>>>>

>>>> It also should be deprecated from the layer in which it currently resides

>>>> and used internally only; it adds no value at the data plane abstraction

>>>> layer. Code bases will already be using their own mechanism for build time

>>>> assertions.

>>>>

>>>>>> +#ifdef __cplusplus

>>>>

>>>> Consistent use of "#if defined(identifier)" advised.

>>>>

>>>>>> +#define static_assert(e, s) \

>>>>>> +       extern int _sassert[(e) ? 1 : -1]

>>>>>> +#else

>>>>>>  #define _Static_assert(e, s) (extern int (*static_assert_checker(void)) \

>>>>>>         [sizeof(struct { unsigned int error_if_negative:(e) ? 1 : -1; })])

>>>>>>

>>>>>>  #endif

>>>>>> +#endif

>>>>>>

>>>>>>  #endif

>>>>>>

>>>>>> @@ -39,9 +46,11 @@ extern "C" {

>>>>>>   * if condition 'cond' is false. Macro definition is empty when compiler is not

>>>>>>   * supported or the compiler does not support static assertion.

>>>>>>   */

>>>>>> -#define ODP_STATIC_ASSERT(cond, msg)  _Static_assert(cond, msg)

>>>>>> +#ifndef __cplusplus

>>>>>> +#define ODP_STATIC_ASSERT(cond, msg) _Static_assert(cond, msg)

>>>>>>

>>>>

>>>> Newline here could be removed.

>>>>

>>>>>> -#ifdef __cplusplus

>>>>>> +#else

>>>>>> +#define ODP_STATIC_ASSERT(cond, msg) static_assert(cond, msg)

>>>>>>  }

>>>>>>  #endif

>>>>>>

>>>>>> diff --git a/test/common_plat/miscellaneous/odp_api_from_cpp.cpp b/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>>>>> index 2b30786..4578ae4 100644

>>>>>> --- a/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>>>>> +++ b/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>>>>> @@ -1,10 +1,9 @@

>>>>>>  #include <cstdio>

>>>>>>  #include <odp_api.h>

>>>>>> -#include <odp/helper/threads.h>

>>>>>> +#include <odp/helper/odph_api.h>

>>>>>>

>>>>>>  int main(int argc ODP_UNUSED, const char *argv[] ODP_UNUSED)

>>>>>>  {

>>>>>> -

>>>>>>         printf("\tODP API version: %s\n", odp_version_api_str());

>>>>>>         printf("\tODP implementation version: %s\n", odp_version_impl_str());

>>>>>>

>>>>>> --

>>>>>> 2.7.4

>>>>>>

>>>

>
Bill Fischofer Feb. 8, 2017, 1:04 a.m. | #7
I've done a fair amount of experimentation with using variants of
Brian's suggested macros. This works for C but for C++ it results in
an error message saying that it can't do relocation and to please
compile with -fPIC, which we've already determined is an unacceptable
requirement for ODP programs. These sort of issues are why
_Static_assert() and static_assert() were introduced to the languages
in the first place.

Can we require -std=c++11 when using ODP?

On Tue, Feb 7, 2017 at 8:42 AM, Bill Fischofer
<bill.fischofer@linaro.org> wrote:
> Given these issues with obsolete compiler levels, it may be simpler to

> go with Brian's suggestion and just use some simple macros. I'll post

> a v4 along those lines.

>

> On Tue, Feb 7, 2017 at 7:51 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

>> On 02/07/17 16:39, Bill Fischofer wrote:

>>> The errors being reported here are on unchanged code paths with an

>>> older clang version (3.4.0 vs.4.2.1). Not sure why you'd see that here

>>> and not in the unchanged code.

>>>

>>> Also, this run is reporting doxygen issues with the traffic manager

>>> that I don't see running locally:

>>>

>>> $ make doxygen-doc

>>>   DXGEN  doc/application-api-guide/Doxyfile

>>> warning: ignoring unsupported tag `HTML_EXTRA_STYLESHEET =' at line

>>> 26, file ./doc/Doxyfile_common

>>> error: Unexpected start tag `services' found in scope='class/memberdecl/'!

>>> error: Unexpected start tag `interfaces' found in scope='class/memberdecl/'!

>>> error: Unexpected start tag `services' found in scope='class/memberdef/'!

>>> error: Unexpected start tag `interfaces' found in scope='class/memberdef/'!

>>> error: Unexpected start tag `constantgroups' found in

>>> scope='namespace/memberdecl/'!

>>> error: Unexpected start tag `constantgroups' found in scope='file/memberdecl/'!

>>> /home/travis/build/muvarov/odp/doc/application-api-guide/api_guide_lines.dox:10:

>>> warning: Found unknown command `\tableofcontents'

>>> /home/travis/build/muvarov/odp/doc/application-api-guide/release.dox:8:

>>> warning: Found unknown command `\tableofcontents'

>>> /home/travis/build/muvarov/odp/include/odp/api/spec/traffic_mngr.h:1717:

>>> warning: expected whitespace after param command

>>> /home/travis/build/muvarov/odp/include/odp/api/spec/traffic_mngr.h:1691:

>>> warning: argument 'inout' of command @param is not found in the

>>> argument list of odp_tm_node_fanin_info(odp_tm_node_t tm_node,

>>> odp_tm_node_fanin_info_t *info)

>>> /home/travis/build/muvarov/odp/include/odp/api/spec/traffic_mngr.h:1691:

>>> warning: The following parameters of

>>> odp_tm_node_fanin_info(odp_tm_node_t tm_node, odp_tm_node_fanin_info_t

>>> *info) are not documented:

>>>   parameter 'info'

>>>   DXGEN  doc/helper-guide/Doxyfile

>>> warning: ignoring unsupported tag `HTML_EXTRA_STYLESHEET =' at line

>>> 31, file ./doc/helper-guide/Doxyfile

>>> error: Unexpected start tag `services' found in scope='class/memberdecl/'!

>>> error: Unexpected start tag `interfaces' found in scope='class/memberdecl/'!

>>> error: Unexpected start tag `services' found in scope='class/memberdef/'!

>>> error: Unexpected start tag `interfaces' found in scope='class/memberdef/'!

>>> error: Unexpected start tag `constantgroups' found in

>>> scope='namespace/memberdecl/'!

>>> error: Unexpected start tag `constantgroups' found in scope='file/memberdecl/'!

>>>

>>> So do we also have a doxygen version issue on these systems?

>>>

>>

>>

>> looks like yes. I also do not see comment locally.

>>

>> we also need some way to return non 0 if doxygen reported an error.

>>

>> The command "make doxygen-doc" exited with 0.

>>

>>

>>

>>> On Tue, Feb 7, 2017 at 7:30 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

>>>> I pushed it to github and test failed:

>>>>

>>>> https://api.travis-ci.org/jobs/199201799/log.txt?deansi=true

>>>>

>>>> Maxim.

>>>>

>>>>

>>>> On 02/07/17 05:51, Brian Brooks wrote:

>>>>> On 02/06 13:14:17, Bill Fischofer wrote:

>>>>>> Ping. Brian can you please verify that this now works on your system.

>>>>>> It works on my Ubuntu 16.04 system.

>>>>>

>>>>> Just built on:

>>>>>

>>>>> Ubuntu 16.10 - x86_64

>>>>> Ubuntu 16.04 - aarch64

>>>>> Arch         - aarch64 & x86_64

>>>>>

>>>>> so the previous build error I reported seems to have been resolved.

>>>>>

>>>>>> A review would also be nice. :)

>>>>>

>>>>> LGTM, please...

>>>>>

>>>>> Reviewed-and-tested-by: Brian Brooks <brian.brooks@linaro.org>

>>>>>

>>>>>> On Thu, Feb 2, 2017 at 4:51 PM, Bill Fischofer

>>>>>> <bill.fischofer@linaro.org> wrote:

>>>>>>> The ODP_STATIC_ASSERT() macro expands to _Static_assert(), however when

>>>>>>> used in C++ programs this needs to expand to static_assert().

>>>>>>>

>>>>>>> This resolves Bug https://bugs.linaro.org/show_bug.cgi?id=2852

>>>>>>>

>>>>>>> Reported-by: Moshe Tubul <moshe.tubul@firmitas-cs.com>

>>>>>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>

>>>>>>> ---

>>>>>>> Changes for v4:

>>>>>>> - Remove extraneous debug code

>>>>>>>

>>>>>>> Changes for v3:

>>>>>>> - Support older versions of g++ that don't allow c++11 static assert by default

>>>>>>>

>>>>>>> Changes for v2:

>>>>>>> - Update C++ test case to include helper apis to validate this fix

>>>>>>>

>>>>>>>  platform/linux-generic/include/odp/api/debug.h      | 21 +++++++++++++++------

>>>>>>>  test/common_plat/miscellaneous/odp_api_from_cpp.cpp |  3 +--

>>>>>>>  2 files changed, 16 insertions(+), 8 deletions(-)

>>>>>>>

>>>>>>> diff --git a/platform/linux-generic/include/odp/api/debug.h b/platform/linux-generic/include/odp/api/debug.h

>>>>>>> index 7db1433..44ca5b7 100644

>>>>>>> --- a/platform/linux-generic/include/odp/api/debug.h

>>>>>>> +++ b/platform/linux-generic/include/odp/api/debug.h

>>>>>>> @@ -19,18 +19,25 @@ extern "C" {

>>>>>>>

>>>>>>>  #include <odp/api/spec/debug.h>

>>>>>>>

>>>>>>> -#if defined(__GNUC__) && !defined(__clang__)

>>>>>>> +#if defined(__GNUC__)

>>>>>>>

>>>>>>> -#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6))

>>>>>>> +#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6)) || \

>>>>>>> +       (__GNUC__ < 6 && defined(__cplusplus))

>>>>>>>

>>>>>>>  /**

>>>>>>> - * @internal _Static_assert was only added in GCC 4.6. Provide a weak replacement

>>>>>>> - * for previous versions.

>>>>>>> + * @internal _Static_assert was only added in GCC 4.6 and the C++ version

>>>>>>> + * static_assert for g++ 6 and above.  Provide a weak replacement for previous

>>>>>>> + * versions.

>>>>>>>   */

>>>>>

>>>>> I think a plain C-style build time assertion (no special compiler support

>>>>> needed - besides unused attribute) will work for C++ as well. So the entire

>>>>> thing could just be:

>>>>>

>>>>>   #define _SA1(cond_, line_) \

>>>>>     extern int __static_assertion_ ## line_[1 - 2*!(cond_)] __unused

>>>>>   #define _SA0(cond_, line_) _SA1(cond_, line_)

>>>>>   #define ODP_STATIC_ASSERT(cond) _SA0(cond, __LINE__)

>>>>>

>>>>> It also should be deprecated from the layer in which it currently resides

>>>>> and used internally only; it adds no value at the data plane abstraction

>>>>> layer. Code bases will already be using their own mechanism for build time

>>>>> assertions.

>>>>>

>>>>>>> +#ifdef __cplusplus

>>>>>

>>>>> Consistent use of "#if defined(identifier)" advised.

>>>>>

>>>>>>> +#define static_assert(e, s) \

>>>>>>> +       extern int _sassert[(e) ? 1 : -1]

>>>>>>> +#else

>>>>>>>  #define _Static_assert(e, s) (extern int (*static_assert_checker(void)) \

>>>>>>>         [sizeof(struct { unsigned int error_if_negative:(e) ? 1 : -1; })])

>>>>>>>

>>>>>>>  #endif

>>>>>>> +#endif

>>>>>>>

>>>>>>>  #endif

>>>>>>>

>>>>>>> @@ -39,9 +46,11 @@ extern "C" {

>>>>>>>   * if condition 'cond' is false. Macro definition is empty when compiler is not

>>>>>>>   * supported or the compiler does not support static assertion.

>>>>>>>   */

>>>>>>> -#define ODP_STATIC_ASSERT(cond, msg)  _Static_assert(cond, msg)

>>>>>>> +#ifndef __cplusplus

>>>>>>> +#define ODP_STATIC_ASSERT(cond, msg) _Static_assert(cond, msg)

>>>>>>>

>>>>>

>>>>> Newline here could be removed.

>>>>>

>>>>>>> -#ifdef __cplusplus

>>>>>>> +#else

>>>>>>> +#define ODP_STATIC_ASSERT(cond, msg) static_assert(cond, msg)

>>>>>>>  }

>>>>>>>  #endif

>>>>>>>

>>>>>>> diff --git a/test/common_plat/miscellaneous/odp_api_from_cpp.cpp b/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>>>>>> index 2b30786..4578ae4 100644

>>>>>>> --- a/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>>>>>> +++ b/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>>>>>> @@ -1,10 +1,9 @@

>>>>>>>  #include <cstdio>

>>>>>>>  #include <odp_api.h>

>>>>>>> -#include <odp/helper/threads.h>

>>>>>>> +#include <odp/helper/odph_api.h>

>>>>>>>

>>>>>>>  int main(int argc ODP_UNUSED, const char *argv[] ODP_UNUSED)

>>>>>>>  {

>>>>>>> -

>>>>>>>         printf("\tODP API version: %s\n", odp_version_api_str());

>>>>>>>         printf("\tODP implementation version: %s\n", odp_version_impl_str());

>>>>>>>

>>>>>>> --

>>>>>>> 2.7.4

>>>>>>>

>>>>

>>
Maxim Uvarov March 31, 2017, 3:02 p.m. | #8
rechecked this patch and it fails:
https://travis-ci.org/muvarov/odp/jobs/217195437

Maxim.

On 02/08/17 04:04, Bill Fischofer wrote:
> I've done a fair amount of experimentation with using variants of

> Brian's suggested macros. This works for C but for C++ it results in

> an error message saying that it can't do relocation and to please

> compile with -fPIC, which we've already determined is an unacceptable

> requirement for ODP programs. These sort of issues are why

> _Static_assert() and static_assert() were introduced to the languages

> in the first place.

> 

> Can we require -std=c++11 when using ODP?

> 

> On Tue, Feb 7, 2017 at 8:42 AM, Bill Fischofer

> <bill.fischofer@linaro.org> wrote:

>> Given these issues with obsolete compiler levels, it may be simpler to

>> go with Brian's suggestion and just use some simple macros. I'll post

>> a v4 along those lines.

>>

>> On Tue, Feb 7, 2017 at 7:51 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

>>> On 02/07/17 16:39, Bill Fischofer wrote:

>>>> The errors being reported here are on unchanged code paths with an

>>>> older clang version (3.4.0 vs.4.2.1). Not sure why you'd see that here

>>>> and not in the unchanged code.

>>>>

>>>> Also, this run is reporting doxygen issues with the traffic manager

>>>> that I don't see running locally:

>>>>

>>>> $ make doxygen-doc

>>>>   DXGEN  doc/application-api-guide/Doxyfile

>>>> warning: ignoring unsupported tag `HTML_EXTRA_STYLESHEET =' at line

>>>> 26, file ./doc/Doxyfile_common

>>>> error: Unexpected start tag `services' found in scope='class/memberdecl/'!

>>>> error: Unexpected start tag `interfaces' found in scope='class/memberdecl/'!

>>>> error: Unexpected start tag `services' found in scope='class/memberdef/'!

>>>> error: Unexpected start tag `interfaces' found in scope='class/memberdef/'!

>>>> error: Unexpected start tag `constantgroups' found in

>>>> scope='namespace/memberdecl/'!

>>>> error: Unexpected start tag `constantgroups' found in scope='file/memberdecl/'!

>>>> /home/travis/build/muvarov/odp/doc/application-api-guide/api_guide_lines.dox:10:

>>>> warning: Found unknown command `\tableofcontents'

>>>> /home/travis/build/muvarov/odp/doc/application-api-guide/release.dox:8:

>>>> warning: Found unknown command `\tableofcontents'

>>>> /home/travis/build/muvarov/odp/include/odp/api/spec/traffic_mngr.h:1717:

>>>> warning: expected whitespace after param command

>>>> /home/travis/build/muvarov/odp/include/odp/api/spec/traffic_mngr.h:1691:

>>>> warning: argument 'inout' of command @param is not found in the

>>>> argument list of odp_tm_node_fanin_info(odp_tm_node_t tm_node,

>>>> odp_tm_node_fanin_info_t *info)

>>>> /home/travis/build/muvarov/odp/include/odp/api/spec/traffic_mngr.h:1691:

>>>> warning: The following parameters of

>>>> odp_tm_node_fanin_info(odp_tm_node_t tm_node, odp_tm_node_fanin_info_t

>>>> *info) are not documented:

>>>>   parameter 'info'

>>>>   DXGEN  doc/helper-guide/Doxyfile

>>>> warning: ignoring unsupported tag `HTML_EXTRA_STYLESHEET =' at line

>>>> 31, file ./doc/helper-guide/Doxyfile

>>>> error: Unexpected start tag `services' found in scope='class/memberdecl/'!

>>>> error: Unexpected start tag `interfaces' found in scope='class/memberdecl/'!

>>>> error: Unexpected start tag `services' found in scope='class/memberdef/'!

>>>> error: Unexpected start tag `interfaces' found in scope='class/memberdef/'!

>>>> error: Unexpected start tag `constantgroups' found in

>>>> scope='namespace/memberdecl/'!

>>>> error: Unexpected start tag `constantgroups' found in scope='file/memberdecl/'!

>>>>

>>>> So do we also have a doxygen version issue on these systems?

>>>>

>>>

>>>

>>> looks like yes. I also do not see comment locally.

>>>

>>> we also need some way to return non 0 if doxygen reported an error.

>>>

>>> The command "make doxygen-doc" exited with 0.

>>>

>>>

>>>

>>>> On Tue, Feb 7, 2017 at 7:30 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

>>>>> I pushed it to github and test failed:

>>>>>

>>>>> https://api.travis-ci.org/jobs/199201799/log.txt?deansi=true

>>>>>

>>>>> Maxim.

>>>>>

>>>>>

>>>>> On 02/07/17 05:51, Brian Brooks wrote:

>>>>>> On 02/06 13:14:17, Bill Fischofer wrote:

>>>>>>> Ping. Brian can you please verify that this now works on your system.

>>>>>>> It works on my Ubuntu 16.04 system.

>>>>>>

>>>>>> Just built on:

>>>>>>

>>>>>> Ubuntu 16.10 - x86_64

>>>>>> Ubuntu 16.04 - aarch64

>>>>>> Arch         - aarch64 & x86_64

>>>>>>

>>>>>> so the previous build error I reported seems to have been resolved.

>>>>>>

>>>>>>> A review would also be nice. :)

>>>>>>

>>>>>> LGTM, please...

>>>>>>

>>>>>> Reviewed-and-tested-by: Brian Brooks <brian.brooks@linaro.org>

>>>>>>

>>>>>>> On Thu, Feb 2, 2017 at 4:51 PM, Bill Fischofer

>>>>>>> <bill.fischofer@linaro.org> wrote:

>>>>>>>> The ODP_STATIC_ASSERT() macro expands to _Static_assert(), however when

>>>>>>>> used in C++ programs this needs to expand to static_assert().

>>>>>>>>

>>>>>>>> This resolves Bug https://bugs.linaro.org/show_bug.cgi?id=2852

>>>>>>>>

>>>>>>>> Reported-by: Moshe Tubul <moshe.tubul@firmitas-cs.com>

>>>>>>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>

>>>>>>>> ---

>>>>>>>> Changes for v4:

>>>>>>>> - Remove extraneous debug code

>>>>>>>>

>>>>>>>> Changes for v3:

>>>>>>>> - Support older versions of g++ that don't allow c++11 static assert by default

>>>>>>>>

>>>>>>>> Changes for v2:

>>>>>>>> - Update C++ test case to include helper apis to validate this fix

>>>>>>>>

>>>>>>>>  platform/linux-generic/include/odp/api/debug.h      | 21 +++++++++++++++------

>>>>>>>>  test/common_plat/miscellaneous/odp_api_from_cpp.cpp |  3 +--

>>>>>>>>  2 files changed, 16 insertions(+), 8 deletions(-)

>>>>>>>>

>>>>>>>> diff --git a/platform/linux-generic/include/odp/api/debug.h b/platform/linux-generic/include/odp/api/debug.h

>>>>>>>> index 7db1433..44ca5b7 100644

>>>>>>>> --- a/platform/linux-generic/include/odp/api/debug.h

>>>>>>>> +++ b/platform/linux-generic/include/odp/api/debug.h

>>>>>>>> @@ -19,18 +19,25 @@ extern "C" {

>>>>>>>>

>>>>>>>>  #include <odp/api/spec/debug.h>

>>>>>>>>

>>>>>>>> -#if defined(__GNUC__) && !defined(__clang__)

>>>>>>>> +#if defined(__GNUC__)

>>>>>>>>

>>>>>>>> -#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6))

>>>>>>>> +#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6)) || \

>>>>>>>> +       (__GNUC__ < 6 && defined(__cplusplus))

>>>>>>>>

>>>>>>>>  /**

>>>>>>>> - * @internal _Static_assert was only added in GCC 4.6. Provide a weak replacement

>>>>>>>> - * for previous versions.

>>>>>>>> + * @internal _Static_assert was only added in GCC 4.6 and the C++ version

>>>>>>>> + * static_assert for g++ 6 and above.  Provide a weak replacement for previous

>>>>>>>> + * versions.

>>>>>>>>   */

>>>>>>

>>>>>> I think a plain C-style build time assertion (no special compiler support

>>>>>> needed - besides unused attribute) will work for C++ as well. So the entire

>>>>>> thing could just be:

>>>>>>

>>>>>>   #define _SA1(cond_, line_) \

>>>>>>     extern int __static_assertion_ ## line_[1 - 2*!(cond_)] __unused

>>>>>>   #define _SA0(cond_, line_) _SA1(cond_, line_)

>>>>>>   #define ODP_STATIC_ASSERT(cond) _SA0(cond, __LINE__)

>>>>>>

>>>>>> It also should be deprecated from the layer in which it currently resides

>>>>>> and used internally only; it adds no value at the data plane abstraction

>>>>>> layer. Code bases will already be using their own mechanism for build time

>>>>>> assertions.

>>>>>>

>>>>>>>> +#ifdef __cplusplus

>>>>>>

>>>>>> Consistent use of "#if defined(identifier)" advised.

>>>>>>

>>>>>>>> +#define static_assert(e, s) \

>>>>>>>> +       extern int _sassert[(e) ? 1 : -1]

>>>>>>>> +#else

>>>>>>>>  #define _Static_assert(e, s) (extern int (*static_assert_checker(void)) \

>>>>>>>>         [sizeof(struct { unsigned int error_if_negative:(e) ? 1 : -1; })])

>>>>>>>>

>>>>>>>>  #endif

>>>>>>>> +#endif

>>>>>>>>

>>>>>>>>  #endif

>>>>>>>>

>>>>>>>> @@ -39,9 +46,11 @@ extern "C" {

>>>>>>>>   * if condition 'cond' is false. Macro definition is empty when compiler is not

>>>>>>>>   * supported or the compiler does not support static assertion.

>>>>>>>>   */

>>>>>>>> -#define ODP_STATIC_ASSERT(cond, msg)  _Static_assert(cond, msg)

>>>>>>>> +#ifndef __cplusplus

>>>>>>>> +#define ODP_STATIC_ASSERT(cond, msg) _Static_assert(cond, msg)

>>>>>>>>

>>>>>>

>>>>>> Newline here could be removed.

>>>>>>

>>>>>>>> -#ifdef __cplusplus

>>>>>>>> +#else

>>>>>>>> +#define ODP_STATIC_ASSERT(cond, msg) static_assert(cond, msg)

>>>>>>>>  }

>>>>>>>>  #endif

>>>>>>>>

>>>>>>>> diff --git a/test/common_plat/miscellaneous/odp_api_from_cpp.cpp b/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>>>>>>> index 2b30786..4578ae4 100644

>>>>>>>> --- a/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>>>>>>> +++ b/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>>>>>>> @@ -1,10 +1,9 @@

>>>>>>>>  #include <cstdio>

>>>>>>>>  #include <odp_api.h>

>>>>>>>> -#include <odp/helper/threads.h>

>>>>>>>> +#include <odp/helper/odph_api.h>

>>>>>>>>

>>>>>>>>  int main(int argc ODP_UNUSED, const char *argv[] ODP_UNUSED)

>>>>>>>>  {

>>>>>>>> -

>>>>>>>>         printf("\tODP API version: %s\n", odp_version_api_str());

>>>>>>>>         printf("\tODP implementation version: %s\n", odp_version_impl_str());

>>>>>>>>

>>>>>>>> --

>>>>>>>> 2.7.4

>>>>>>>>

>>>>>

>>>
Brian Brooks April 12, 2017, 6:58 p.m. | #9
On Fri, Mar 31, 2017 at 10:02 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> rechecked this patch and it fails:

> https://travis-ci.org/muvarov/odp/jobs/217195437

>

> Maxim.

>

> On 02/08/17 04:04, Bill Fischofer wrote:

>> I've done a fair amount of experimentation with using variants of

>> Brian's suggested macros. This works for C but for C++ it results in

>> an error message saying that it can't do relocation and to please

>> compile with -fPIC, which we've already determined is an unacceptable

>> requirement for ODP programs. These sort of issues are why

>> _Static_assert() and static_assert() were introduced to the languages

>> in the first place.

>>

>> Can we require -std=c++11 when using ODP?


Would it be possible to move this macro to internal-only where C is
used? The problem goes away if we are allowed to lower it. I don't
know why such a macro exists at the top-level API to begin with.

>> On Tue, Feb 7, 2017 at 8:42 AM, Bill Fischofer

>> <bill.fischofer@linaro.org> wrote:

>>> Given these issues with obsolete compiler levels, it may be simpler to

>>> go with Brian's suggestion and just use some simple macros. I'll post

>>> a v4 along those lines.

>>>

>>> On Tue, Feb 7, 2017 at 7:51 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

>>>> On 02/07/17 16:39, Bill Fischofer wrote:

>>>>> The errors being reported here are on unchanged code paths with an

>>>>> older clang version (3.4.0 vs.4.2.1). Not sure why you'd see that here

>>>>> and not in the unchanged code.

>>>>>

>>>>> Also, this run is reporting doxygen issues with the traffic manager

>>>>> that I don't see running locally:

>>>>>

>>>>> $ make doxygen-doc

>>>>>   DXGEN  doc/application-api-guide/Doxyfile

>>>>> warning: ignoring unsupported tag `HTML_EXTRA_STYLESHEET =' at line

>>>>> 26, file ./doc/Doxyfile_common

>>>>> error: Unexpected start tag `services' found in scope='class/memberdecl/'!

>>>>> error: Unexpected start tag `interfaces' found in scope='class/memberdecl/'!

>>>>> error: Unexpected start tag `services' found in scope='class/memberdef/'!

>>>>> error: Unexpected start tag `interfaces' found in scope='class/memberdef/'!

>>>>> error: Unexpected start tag `constantgroups' found in

>>>>> scope='namespace/memberdecl/'!

>>>>> error: Unexpected start tag `constantgroups' found in scope='file/memberdecl/'!

>>>>> /home/travis/build/muvarov/odp/doc/application-api-guide/api_guide_lines.dox:10:

>>>>> warning: Found unknown command `\tableofcontents'

>>>>> /home/travis/build/muvarov/odp/doc/application-api-guide/release.dox:8:

>>>>> warning: Found unknown command `\tableofcontents'

>>>>> /home/travis/build/muvarov/odp/include/odp/api/spec/traffic_mngr.h:1717:

>>>>> warning: expected whitespace after param command

>>>>> /home/travis/build/muvarov/odp/include/odp/api/spec/traffic_mngr.h:1691:

>>>>> warning: argument 'inout' of command @param is not found in the

>>>>> argument list of odp_tm_node_fanin_info(odp_tm_node_t tm_node,

>>>>> odp_tm_node_fanin_info_t *info)

>>>>> /home/travis/build/muvarov/odp/include/odp/api/spec/traffic_mngr.h:1691:

>>>>> warning: The following parameters of

>>>>> odp_tm_node_fanin_info(odp_tm_node_t tm_node, odp_tm_node_fanin_info_t

>>>>> *info) are not documented:

>>>>>   parameter 'info'

>>>>>   DXGEN  doc/helper-guide/Doxyfile

>>>>> warning: ignoring unsupported tag `HTML_EXTRA_STYLESHEET =' at line

>>>>> 31, file ./doc/helper-guide/Doxyfile

>>>>> error: Unexpected start tag `services' found in scope='class/memberdecl/'!

>>>>> error: Unexpected start tag `interfaces' found in scope='class/memberdecl/'!

>>>>> error: Unexpected start tag `services' found in scope='class/memberdef/'!

>>>>> error: Unexpected start tag `interfaces' found in scope='class/memberdef/'!

>>>>> error: Unexpected start tag `constantgroups' found in

>>>>> scope='namespace/memberdecl/'!

>>>>> error: Unexpected start tag `constantgroups' found in scope='file/memberdecl/'!

>>>>>

>>>>> So do we also have a doxygen version issue on these systems?

>>>>>

>>>>

>>>>

>>>> looks like yes. I also do not see comment locally.

>>>>

>>>> we also need some way to return non 0 if doxygen reported an error.

>>>>

>>>> The command "make doxygen-doc" exited with 0.

>>>>

>>>>

>>>>

>>>>> On Tue, Feb 7, 2017 at 7:30 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

>>>>>> I pushed it to github and test failed:

>>>>>>

>>>>>> https://api.travis-ci.org/jobs/199201799/log.txt?deansi=true

>>>>>>

>>>>>> Maxim.

>>>>>>

>>>>>>

>>>>>> On 02/07/17 05:51, Brian Brooks wrote:

>>>>>>> On 02/06 13:14:17, Bill Fischofer wrote:

>>>>>>>> Ping. Brian can you please verify that this now works on your system.

>>>>>>>> It works on my Ubuntu 16.04 system.

>>>>>>>

>>>>>>> Just built on:

>>>>>>>

>>>>>>> Ubuntu 16.10 - x86_64

>>>>>>> Ubuntu 16.04 - aarch64

>>>>>>> Arch         - aarch64 & x86_64

>>>>>>>

>>>>>>> so the previous build error I reported seems to have been resolved.

>>>>>>>

>>>>>>>> A review would also be nice. :)

>>>>>>>

>>>>>>> LGTM, please...

>>>>>>>

>>>>>>> Reviewed-and-tested-by: Brian Brooks <brian.brooks@linaro.org>

>>>>>>>

>>>>>>>> On Thu, Feb 2, 2017 at 4:51 PM, Bill Fischofer

>>>>>>>> <bill.fischofer@linaro.org> wrote:

>>>>>>>>> The ODP_STATIC_ASSERT() macro expands to _Static_assert(), however when

>>>>>>>>> used in C++ programs this needs to expand to static_assert().

>>>>>>>>>

>>>>>>>>> This resolves Bug https://bugs.linaro.org/show_bug.cgi?id=2852

>>>>>>>>>

>>>>>>>>> Reported-by: Moshe Tubul <moshe.tubul@firmitas-cs.com>

>>>>>>>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>

>>>>>>>>> ---

>>>>>>>>> Changes for v4:

>>>>>>>>> - Remove extraneous debug code

>>>>>>>>>

>>>>>>>>> Changes for v3:

>>>>>>>>> - Support older versions of g++ that don't allow c++11 static assert by default

>>>>>>>>>

>>>>>>>>> Changes for v2:

>>>>>>>>> - Update C++ test case to include helper apis to validate this fix

>>>>>>>>>

>>>>>>>>>  platform/linux-generic/include/odp/api/debug.h      | 21 +++++++++++++++------

>>>>>>>>>  test/common_plat/miscellaneous/odp_api_from_cpp.cpp |  3 +--

>>>>>>>>>  2 files changed, 16 insertions(+), 8 deletions(-)

>>>>>>>>>

>>>>>>>>> diff --git a/platform/linux-generic/include/odp/api/debug.h b/platform/linux-generic/include/odp/api/debug.h

>>>>>>>>> index 7db1433..44ca5b7 100644

>>>>>>>>> --- a/platform/linux-generic/include/odp/api/debug.h

>>>>>>>>> +++ b/platform/linux-generic/include/odp/api/debug.h

>>>>>>>>> @@ -19,18 +19,25 @@ extern "C" {

>>>>>>>>>

>>>>>>>>>  #include <odp/api/spec/debug.h>

>>>>>>>>>

>>>>>>>>> -#if defined(__GNUC__) && !defined(__clang__)

>>>>>>>>> +#if defined(__GNUC__)

>>>>>>>>>

>>>>>>>>> -#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6))

>>>>>>>>> +#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6)) || \

>>>>>>>>> +       (__GNUC__ < 6 && defined(__cplusplus))

>>>>>>>>>

>>>>>>>>>  /**

>>>>>>>>> - * @internal _Static_assert was only added in GCC 4.6. Provide a weak replacement

>>>>>>>>> - * for previous versions.

>>>>>>>>> + * @internal _Static_assert was only added in GCC 4.6 and the C++ version

>>>>>>>>> + * static_assert for g++ 6 and above.  Provide a weak replacement for previous

>>>>>>>>> + * versions.

>>>>>>>>>   */

>>>>>>>

>>>>>>> I think a plain C-style build time assertion (no special compiler support

>>>>>>> needed - besides unused attribute) will work for C++ as well. So the entire

>>>>>>> thing could just be:

>>>>>>>

>>>>>>>   #define _SA1(cond_, line_) \

>>>>>>>     extern int __static_assertion_ ## line_[1 - 2*!(cond_)] __unused

>>>>>>>   #define _SA0(cond_, line_) _SA1(cond_, line_)

>>>>>>>   #define ODP_STATIC_ASSERT(cond) _SA0(cond, __LINE__)

>>>>>>>

>>>>>>> It also should be deprecated from the layer in which it currently resides

>>>>>>> and used internally only; it adds no value at the data plane abstraction

>>>>>>> layer. Code bases will already be using their own mechanism for build time

>>>>>>> assertions.

>>>>>>>

>>>>>>>>> +#ifdef __cplusplus

>>>>>>>

>>>>>>> Consistent use of "#if defined(identifier)" advised.

>>>>>>>

>>>>>>>>> +#define static_assert(e, s) \

>>>>>>>>> +       extern int _sassert[(e) ? 1 : -1]

>>>>>>>>> +#else

>>>>>>>>>  #define _Static_assert(e, s) (extern int (*static_assert_checker(void)) \

>>>>>>>>>         [sizeof(struct { unsigned int error_if_negative:(e) ? 1 : -1; })])

>>>>>>>>>

>>>>>>>>>  #endif

>>>>>>>>> +#endif

>>>>>>>>>

>>>>>>>>>  #endif

>>>>>>>>>

>>>>>>>>> @@ -39,9 +46,11 @@ extern "C" {

>>>>>>>>>   * if condition 'cond' is false. Macro definition is empty when compiler is not

>>>>>>>>>   * supported or the compiler does not support static assertion.

>>>>>>>>>   */

>>>>>>>>> -#define ODP_STATIC_ASSERT(cond, msg)  _Static_assert(cond, msg)

>>>>>>>>> +#ifndef __cplusplus

>>>>>>>>> +#define ODP_STATIC_ASSERT(cond, msg) _Static_assert(cond, msg)

>>>>>>>>>

>>>>>>>

>>>>>>> Newline here could be removed.

>>>>>>>

>>>>>>>>> -#ifdef __cplusplus

>>>>>>>>> +#else

>>>>>>>>> +#define ODP_STATIC_ASSERT(cond, msg) static_assert(cond, msg)

>>>>>>>>>  }

>>>>>>>>>  #endif

>>>>>>>>>

>>>>>>>>> diff --git a/test/common_plat/miscellaneous/odp_api_from_cpp.cpp b/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>>>>>>>> index 2b30786..4578ae4 100644

>>>>>>>>> --- a/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>>>>>>>> +++ b/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>>>>>>>> @@ -1,10 +1,9 @@

>>>>>>>>>  #include <cstdio>

>>>>>>>>>  #include <odp_api.h>

>>>>>>>>> -#include <odp/helper/threads.h>

>>>>>>>>> +#include <odp/helper/odph_api.h>

>>>>>>>>>

>>>>>>>>>  int main(int argc ODP_UNUSED, const char *argv[] ODP_UNUSED)

>>>>>>>>>  {

>>>>>>>>> -

>>>>>>>>>         printf("\tODP API version: %s\n", odp_version_api_str());

>>>>>>>>>         printf("\tODP implementation version: %s\n", odp_version_impl_str());

>>>>>>>>>

>>>>>>>>> --

>>>>>>>>> 2.7.4

>>>>>>>>>

>>>>>>

>>>>

>
Bill Fischofer April 12, 2017, 7:06 p.m. | #10
So what's changed between now and when Travis reported this passing
for me? See https://travis-ci.org/Linaro/odp/builds/217908015

On Wed, Apr 12, 2017 at 1:58 PM, Brian Brooks <brian.brooks@linaro.org> wrote:
> On Fri, Mar 31, 2017 at 10:02 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

>> rechecked this patch and it fails:

>> https://travis-ci.org/muvarov/odp/jobs/217195437

>>

>> Maxim.

>>

>> On 02/08/17 04:04, Bill Fischofer wrote:

>>> I've done a fair amount of experimentation with using variants of

>>> Brian's suggested macros. This works for C but for C++ it results in

>>> an error message saying that it can't do relocation and to please

>>> compile with -fPIC, which we've already determined is an unacceptable

>>> requirement for ODP programs. These sort of issues are why

>>> _Static_assert() and static_assert() were introduced to the languages

>>> in the first place.

>>>

>>> Can we require -std=c++11 when using ODP?

>

> Would it be possible to move this macro to internal-only where C is

> used? The problem goes away if we are allowed to lower it. I don't

> know why such a macro exists at the top-level API to begin with.

>

>>> On Tue, Feb 7, 2017 at 8:42 AM, Bill Fischofer

>>> <bill.fischofer@linaro.org> wrote:

>>>> Given these issues with obsolete compiler levels, it may be simpler to

>>>> go with Brian's suggestion and just use some simple macros. I'll post

>>>> a v4 along those lines.

>>>>

>>>> On Tue, Feb 7, 2017 at 7:51 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

>>>>> On 02/07/17 16:39, Bill Fischofer wrote:

>>>>>> The errors being reported here are on unchanged code paths with an

>>>>>> older clang version (3.4.0 vs.4.2.1). Not sure why you'd see that here

>>>>>> and not in the unchanged code.

>>>>>>

>>>>>> Also, this run is reporting doxygen issues with the traffic manager

>>>>>> that I don't see running locally:

>>>>>>

>>>>>> $ make doxygen-doc

>>>>>>   DXGEN  doc/application-api-guide/Doxyfile

>>>>>> warning: ignoring unsupported tag `HTML_EXTRA_STYLESHEET =' at line

>>>>>> 26, file ./doc/Doxyfile_common

>>>>>> error: Unexpected start tag `services' found in scope='class/memberdecl/'!

>>>>>> error: Unexpected start tag `interfaces' found in scope='class/memberdecl/'!

>>>>>> error: Unexpected start tag `services' found in scope='class/memberdef/'!

>>>>>> error: Unexpected start tag `interfaces' found in scope='class/memberdef/'!

>>>>>> error: Unexpected start tag `constantgroups' found in

>>>>>> scope='namespace/memberdecl/'!

>>>>>> error: Unexpected start tag `constantgroups' found in scope='file/memberdecl/'!

>>>>>> /home/travis/build/muvarov/odp/doc/application-api-guide/api_guide_lines.dox:10:

>>>>>> warning: Found unknown command `\tableofcontents'

>>>>>> /home/travis/build/muvarov/odp/doc/application-api-guide/release.dox:8:

>>>>>> warning: Found unknown command `\tableofcontents'

>>>>>> /home/travis/build/muvarov/odp/include/odp/api/spec/traffic_mngr.h:1717:

>>>>>> warning: expected whitespace after param command

>>>>>> /home/travis/build/muvarov/odp/include/odp/api/spec/traffic_mngr.h:1691:

>>>>>> warning: argument 'inout' of command @param is not found in the

>>>>>> argument list of odp_tm_node_fanin_info(odp_tm_node_t tm_node,

>>>>>> odp_tm_node_fanin_info_t *info)

>>>>>> /home/travis/build/muvarov/odp/include/odp/api/spec/traffic_mngr.h:1691:

>>>>>> warning: The following parameters of

>>>>>> odp_tm_node_fanin_info(odp_tm_node_t tm_node, odp_tm_node_fanin_info_t

>>>>>> *info) are not documented:

>>>>>>   parameter 'info'

>>>>>>   DXGEN  doc/helper-guide/Doxyfile

>>>>>> warning: ignoring unsupported tag `HTML_EXTRA_STYLESHEET =' at line

>>>>>> 31, file ./doc/helper-guide/Doxyfile

>>>>>> error: Unexpected start tag `services' found in scope='class/memberdecl/'!

>>>>>> error: Unexpected start tag `interfaces' found in scope='class/memberdecl/'!

>>>>>> error: Unexpected start tag `services' found in scope='class/memberdef/'!

>>>>>> error: Unexpected start tag `interfaces' found in scope='class/memberdef/'!

>>>>>> error: Unexpected start tag `constantgroups' found in

>>>>>> scope='namespace/memberdecl/'!

>>>>>> error: Unexpected start tag `constantgroups' found in scope='file/memberdecl/'!

>>>>>>

>>>>>> So do we also have a doxygen version issue on these systems?

>>>>>>

>>>>>

>>>>>

>>>>> looks like yes. I also do not see comment locally.

>>>>>

>>>>> we also need some way to return non 0 if doxygen reported an error.

>>>>>

>>>>> The command "make doxygen-doc" exited with 0.

>>>>>

>>>>>

>>>>>

>>>>>> On Tue, Feb 7, 2017 at 7:30 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

>>>>>>> I pushed it to github and test failed:

>>>>>>>

>>>>>>> https://api.travis-ci.org/jobs/199201799/log.txt?deansi=true

>>>>>>>

>>>>>>> Maxim.

>>>>>>>

>>>>>>>

>>>>>>> On 02/07/17 05:51, Brian Brooks wrote:

>>>>>>>> On 02/06 13:14:17, Bill Fischofer wrote:

>>>>>>>>> Ping. Brian can you please verify that this now works on your system.

>>>>>>>>> It works on my Ubuntu 16.04 system.

>>>>>>>>

>>>>>>>> Just built on:

>>>>>>>>

>>>>>>>> Ubuntu 16.10 - x86_64

>>>>>>>> Ubuntu 16.04 - aarch64

>>>>>>>> Arch         - aarch64 & x86_64

>>>>>>>>

>>>>>>>> so the previous build error I reported seems to have been resolved.

>>>>>>>>

>>>>>>>>> A review would also be nice. :)

>>>>>>>>

>>>>>>>> LGTM, please...

>>>>>>>>

>>>>>>>> Reviewed-and-tested-by: Brian Brooks <brian.brooks@linaro.org>

>>>>>>>>

>>>>>>>>> On Thu, Feb 2, 2017 at 4:51 PM, Bill Fischofer

>>>>>>>>> <bill.fischofer@linaro.org> wrote:

>>>>>>>>>> The ODP_STATIC_ASSERT() macro expands to _Static_assert(), however when

>>>>>>>>>> used in C++ programs this needs to expand to static_assert().

>>>>>>>>>>

>>>>>>>>>> This resolves Bug https://bugs.linaro.org/show_bug.cgi?id=2852

>>>>>>>>>>

>>>>>>>>>> Reported-by: Moshe Tubul <moshe.tubul@firmitas-cs.com>

>>>>>>>>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>

>>>>>>>>>> ---

>>>>>>>>>> Changes for v4:

>>>>>>>>>> - Remove extraneous debug code

>>>>>>>>>>

>>>>>>>>>> Changes for v3:

>>>>>>>>>> - Support older versions of g++ that don't allow c++11 static assert by default

>>>>>>>>>>

>>>>>>>>>> Changes for v2:

>>>>>>>>>> - Update C++ test case to include helper apis to validate this fix

>>>>>>>>>>

>>>>>>>>>>  platform/linux-generic/include/odp/api/debug.h      | 21 +++++++++++++++------

>>>>>>>>>>  test/common_plat/miscellaneous/odp_api_from_cpp.cpp |  3 +--

>>>>>>>>>>  2 files changed, 16 insertions(+), 8 deletions(-)

>>>>>>>>>>

>>>>>>>>>> diff --git a/platform/linux-generic/include/odp/api/debug.h b/platform/linux-generic/include/odp/api/debug.h

>>>>>>>>>> index 7db1433..44ca5b7 100644

>>>>>>>>>> --- a/platform/linux-generic/include/odp/api/debug.h

>>>>>>>>>> +++ b/platform/linux-generic/include/odp/api/debug.h

>>>>>>>>>> @@ -19,18 +19,25 @@ extern "C" {

>>>>>>>>>>

>>>>>>>>>>  #include <odp/api/spec/debug.h>

>>>>>>>>>>

>>>>>>>>>> -#if defined(__GNUC__) && !defined(__clang__)

>>>>>>>>>> +#if defined(__GNUC__)

>>>>>>>>>>

>>>>>>>>>> -#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6))

>>>>>>>>>> +#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6)) || \

>>>>>>>>>> +       (__GNUC__ < 6 && defined(__cplusplus))

>>>>>>>>>>

>>>>>>>>>>  /**

>>>>>>>>>> - * @internal _Static_assert was only added in GCC 4.6. Provide a weak replacement

>>>>>>>>>> - * for previous versions.

>>>>>>>>>> + * @internal _Static_assert was only added in GCC 4.6 and the C++ version

>>>>>>>>>> + * static_assert for g++ 6 and above.  Provide a weak replacement for previous

>>>>>>>>>> + * versions.

>>>>>>>>>>   */

>>>>>>>>

>>>>>>>> I think a plain C-style build time assertion (no special compiler support

>>>>>>>> needed - besides unused attribute) will work for C++ as well. So the entire

>>>>>>>> thing could just be:

>>>>>>>>

>>>>>>>>   #define _SA1(cond_, line_) \

>>>>>>>>     extern int __static_assertion_ ## line_[1 - 2*!(cond_)] __unused

>>>>>>>>   #define _SA0(cond_, line_) _SA1(cond_, line_)

>>>>>>>>   #define ODP_STATIC_ASSERT(cond) _SA0(cond, __LINE__)

>>>>>>>>

>>>>>>>> It also should be deprecated from the layer in which it currently resides

>>>>>>>> and used internally only; it adds no value at the data plane abstraction

>>>>>>>> layer. Code bases will already be using their own mechanism for build time

>>>>>>>> assertions.

>>>>>>>>

>>>>>>>>>> +#ifdef __cplusplus

>>>>>>>>

>>>>>>>> Consistent use of "#if defined(identifier)" advised.

>>>>>>>>

>>>>>>>>>> +#define static_assert(e, s) \

>>>>>>>>>> +       extern int _sassert[(e) ? 1 : -1]

>>>>>>>>>> +#else

>>>>>>>>>>  #define _Static_assert(e, s) (extern int (*static_assert_checker(void)) \

>>>>>>>>>>         [sizeof(struct { unsigned int error_if_negative:(e) ? 1 : -1; })])

>>>>>>>>>>

>>>>>>>>>>  #endif

>>>>>>>>>> +#endif

>>>>>>>>>>

>>>>>>>>>>  #endif

>>>>>>>>>>

>>>>>>>>>> @@ -39,9 +46,11 @@ extern "C" {

>>>>>>>>>>   * if condition 'cond' is false. Macro definition is empty when compiler is not

>>>>>>>>>>   * supported or the compiler does not support static assertion.

>>>>>>>>>>   */

>>>>>>>>>> -#define ODP_STATIC_ASSERT(cond, msg)  _Static_assert(cond, msg)

>>>>>>>>>> +#ifndef __cplusplus

>>>>>>>>>> +#define ODP_STATIC_ASSERT(cond, msg) _Static_assert(cond, msg)

>>>>>>>>>>

>>>>>>>>

>>>>>>>> Newline here could be removed.

>>>>>>>>

>>>>>>>>>> -#ifdef __cplusplus

>>>>>>>>>> +#else

>>>>>>>>>> +#define ODP_STATIC_ASSERT(cond, msg) static_assert(cond, msg)

>>>>>>>>>>  }

>>>>>>>>>>  #endif

>>>>>>>>>>

>>>>>>>>>> diff --git a/test/common_plat/miscellaneous/odp_api_from_cpp.cpp b/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>>>>>>>>> index 2b30786..4578ae4 100644

>>>>>>>>>> --- a/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>>>>>>>>> +++ b/test/common_plat/miscellaneous/odp_api_from_cpp.cpp

>>>>>>>>>> @@ -1,10 +1,9 @@

>>>>>>>>>>  #include <cstdio>

>>>>>>>>>>  #include <odp_api.h>

>>>>>>>>>> -#include <odp/helper/threads.h>

>>>>>>>>>> +#include <odp/helper/odph_api.h>

>>>>>>>>>>

>>>>>>>>>>  int main(int argc ODP_UNUSED, const char *argv[] ODP_UNUSED)

>>>>>>>>>>  {

>>>>>>>>>> -

>>>>>>>>>>         printf("\tODP API version: %s\n", odp_version_api_str());

>>>>>>>>>>         printf("\tODP implementation version: %s\n", odp_version_impl_str());

>>>>>>>>>>

>>>>>>>>>> --

>>>>>>>>>> 2.7.4

>>>>>>>>>>

>>>>>>>

>>>>>

>>

Patch

diff --git a/platform/linux-generic/include/odp/api/debug.h b/platform/linux-generic/include/odp/api/debug.h
index 7db1433..44ca5b7 100644
--- a/platform/linux-generic/include/odp/api/debug.h
+++ b/platform/linux-generic/include/odp/api/debug.h
@@ -19,18 +19,25 @@  extern "C" {
 
 #include <odp/api/spec/debug.h>
 
-#if defined(__GNUC__) && !defined(__clang__)
+#if defined(__GNUC__)
 
-#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6))
+#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6)) || \
+	(__GNUC__ < 6 && defined(__cplusplus))
 
 /**
- * @internal _Static_assert was only added in GCC 4.6. Provide a weak replacement
- * for previous versions.
+ * @internal _Static_assert was only added in GCC 4.6 and the C++ version
+ * static_assert for g++ 6 and above.  Provide a weak replacement for previous
+ * versions.
  */
+#ifdef __cplusplus
+#define static_assert(e, s) \
+	extern int _sassert[(e) ? 1 : -1]
+#else
 #define _Static_assert(e, s) (extern int (*static_assert_checker(void)) \
 	[sizeof(struct { unsigned int error_if_negative:(e) ? 1 : -1; })])
 
 #endif
+#endif
 
 #endif
 
@@ -39,9 +46,11 @@  extern "C" {
  * if condition 'cond' is false. Macro definition is empty when compiler is not
  * supported or the compiler does not support static assertion.
  */
-#define ODP_STATIC_ASSERT(cond, msg)  _Static_assert(cond, msg)
+#ifndef __cplusplus
+#define ODP_STATIC_ASSERT(cond, msg) _Static_assert(cond, msg)
 
-#ifdef __cplusplus
+#else
+#define ODP_STATIC_ASSERT(cond, msg) static_assert(cond, msg)
 }
 #endif
 
diff --git a/test/common_plat/miscellaneous/odp_api_from_cpp.cpp b/test/common_plat/miscellaneous/odp_api_from_cpp.cpp
index 2b30786..4578ae4 100644
--- a/test/common_plat/miscellaneous/odp_api_from_cpp.cpp
+++ b/test/common_plat/miscellaneous/odp_api_from_cpp.cpp
@@ -1,10 +1,9 @@ 
 #include <cstdio>
 #include <odp_api.h>
-#include <odp/helper/threads.h>
+#include <odp/helper/odph_api.h>
 
 int main(int argc ODP_UNUSED, const char *argv[] ODP_UNUSED)
 {
-
 	printf("\tODP API version: %s\n", odp_version_api_str());
 	printf("\tODP implementation version: %s\n", odp_version_impl_str());