| Message ID | 1486075915-28921-1-git-send-email-bill.fischofer@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 >
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 > >
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 >>>
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 >>>> >
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 >>>>> >>
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 >>>>>> >>> >
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 >>>>>>> >>>> >>
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 >>>>>>>> >>>>> >>>
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 >>>>>>>>> >>>>>> >>>> >
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 >>>>>>>>>> >>>>>>> >>>>> >>
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());
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