| Message ID | 1421099021-26007-4-git-send-email-mike.holmes@linaro.org |
|---|---|
| State | New |
| Headers | show |
Shouldn't this wait until the coremask patch is in and then be based on it, using the new cpumask names? On Mon, Jan 12, 2015 at 3:43 PM, Mike Holmes <mike.holmes@linaro.org> wrote: > Allow the complier to check for NULL args. > > Signed-off-by: Mike Holmes <mike.holmes@linaro.org> > --- > platform/linux-generic/include/api/odp_coremask.h | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/platform/linux-generic/include/api/odp_coremask.h > b/platform/linux-generic/include/api/odp_coremask.h > index c9331fd..b52f977 100644 > --- a/platform/linux-generic/include/api/odp_coremask.h > +++ b/platform/linux-generic/include/api/odp_coremask.h > @@ -21,6 +21,7 @@ extern "C" { > > > #include <odp_std_types.h> > +#include <odp_hints.h> > > /** @addtogroup odp_scheduler > * Core mask operations. > @@ -51,7 +52,8 @@ typedef struct odp_coremask_t { > * > * @note Supports currently only core indexes upto 63 > */ > -void odp_coremask_from_str(const char *str, odp_coremask_t *mask); > +void odp_coremask_from_str(const char *str, odp_coremask_t *mask) > + ODP_NONNULL(1, 2); > > /** > * Write core mask as a string of hexadecimal digits > @@ -64,7 +66,8 @@ void odp_coremask_from_str(const char *str, > odp_coremask_t *mask); > * > * @note Supports currently only core indexes upto 63 > */ > -void odp_coremask_to_str(char *str, int len, const odp_coremask_t *mask); > +void odp_coremask_to_str(char *str, int len, const odp_coremask_t *mask) > + ODP_NONNULL(1, 3); > > > /** > @@ -87,7 +90,8 @@ void odp_coremask_to_str(char *str, int len, const > odp_coremask_t *mask); > * > * @note Supports currently only core indexes upto 63 > */ > -void odp_coremask_from_u64(const uint64_t *u64, int num, odp_coremask_t > *mask); > +void odp_coremask_from_u64(const uint64_t *u64, int num, odp_coremask_t > *mask) > + ODP_NONNULL(1, 3); > > /** > * Clear entire mask > @@ -103,14 +107,14 @@ static inline void odp_coremask_zero(odp_coremask_t > *mask) > * @param core Core number > * @param mask add core number in core mask > */ > -void odp_coremask_set(int core, odp_coremask_t *mask); > +void odp_coremask_set(int core, odp_coremask_t *mask) ODP_NONNULL(2); > > /** > * Remove core from mask > * @param core Core number > * @param mask clear core number from core mask > */ > -void odp_coremask_clr(int core, odp_coremask_t *mask); > +void odp_coremask_clr(int core, odp_coremask_t *mask) ODP_NONNULL(2); > > /** > * Test if core is a member of mask > @@ -118,14 +122,14 @@ void odp_coremask_clr(int core, odp_coremask_t > *mask); > * @param mask Core mask to check if core num set or not > * @return non-zero if set otherwise 0 > */ > -int odp_coremask_isset(int core, const odp_coremask_t *mask); > +int odp_coremask_isset(int core, const odp_coremask_t *mask) > ODP_NONNULL(2); > > /** > * Count number of cores in mask > * @param mask Core mask > * @return coremask count > */ > -int odp_coremask_count(const odp_coremask_t *mask); > +int odp_coremask_count(const odp_coremask_t *mask) ODP_NONNULL(1); > > > > -- > 2.1.0 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 12 January 2015 at 18:38, Bill Fischofer <bill.fischofer@linaro.org> wrote: > Shouldn't this wait until the coremask patch is in and then be based on > it, using the new cpumask names? > 1/2 Agree. It is in two parts, the important part is the first patch which can be applied independently, the second is an example given as you say that there are other changes afoot since it was written. I hope we accept the first patch, and if the second is not rolled in with Robbies work then core mask will have to be added to the work to fix up all the other APIs to use NONNULL. > On Mon, Jan 12, 2015 at 3:43 PM, Mike Holmes <mike.holmes@linaro.org> > wrote: > >> Allow the complier to check for NULL args. >> >> Signed-off-by: Mike Holmes <mike.holmes@linaro.org> >> --- >> platform/linux-generic/include/api/odp_coremask.h | 18 +++++++++++------- >> 1 file changed, 11 insertions(+), 7 deletions(-) >> >> diff --git a/platform/linux-generic/include/api/odp_coremask.h >> b/platform/linux-generic/include/api/odp_coremask.h >> index c9331fd..b52f977 100644 >> --- a/platform/linux-generic/include/api/odp_coremask.h >> +++ b/platform/linux-generic/include/api/odp_coremask.h >> @@ -21,6 +21,7 @@ extern "C" { >> >> >> #include <odp_std_types.h> >> +#include <odp_hints.h> >> >> /** @addtogroup odp_scheduler >> * Core mask operations. >> @@ -51,7 +52,8 @@ typedef struct odp_coremask_t { >> * >> * @note Supports currently only core indexes upto 63 >> */ >> -void odp_coremask_from_str(const char *str, odp_coremask_t *mask); >> +void odp_coremask_from_str(const char *str, odp_coremask_t *mask) >> + ODP_NONNULL(1, 2); >> >> /** >> * Write core mask as a string of hexadecimal digits >> @@ -64,7 +66,8 @@ void odp_coremask_from_str(const char *str, >> odp_coremask_t *mask); >> * >> * @note Supports currently only core indexes upto 63 >> */ >> -void odp_coremask_to_str(char *str, int len, const odp_coremask_t *mask); >> +void odp_coremask_to_str(char *str, int len, const odp_coremask_t *mask) >> + ODP_NONNULL(1, 3); >> >> >> /** >> @@ -87,7 +90,8 @@ void odp_coremask_to_str(char *str, int len, const >> odp_coremask_t *mask); >> * >> * @note Supports currently only core indexes upto 63 >> */ >> -void odp_coremask_from_u64(const uint64_t *u64, int num, odp_coremask_t >> *mask); >> +void odp_coremask_from_u64(const uint64_t *u64, int num, odp_coremask_t >> *mask) >> + ODP_NONNULL(1, 3); >> >> /** >> * Clear entire mask >> @@ -103,14 +107,14 @@ static inline void odp_coremask_zero(odp_coremask_t >> *mask) >> * @param core Core number >> * @param mask add core number in core mask >> */ >> -void odp_coremask_set(int core, odp_coremask_t *mask); >> +void odp_coremask_set(int core, odp_coremask_t *mask) ODP_NONNULL(2); >> >> /** >> * Remove core from mask >> * @param core Core number >> * @param mask clear core number from core mask >> */ >> -void odp_coremask_clr(int core, odp_coremask_t *mask); >> +void odp_coremask_clr(int core, odp_coremask_t *mask) ODP_NONNULL(2); >> >> /** >> * Test if core is a member of mask >> @@ -118,14 +122,14 @@ void odp_coremask_clr(int core, odp_coremask_t >> *mask); >> * @param mask Core mask to check if core num set or not >> * @return non-zero if set otherwise 0 >> */ >> -int odp_coremask_isset(int core, const odp_coremask_t *mask); >> +int odp_coremask_isset(int core, const odp_coremask_t *mask) >> ODP_NONNULL(2); >> >> /** >> * Count number of cores in mask >> * @param mask Core mask >> * @return coremask count >> */ >> -int odp_coremask_count(const odp_coremask_t *mask); >> +int odp_coremask_count(const odp_coremask_t *mask) ODP_NONNULL(1); >> >> >> >> -- >> 2.1.0 >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp >> > >
On 13 January 2015 at 09:45, Savolainen, Petri (NSN - FI/Espoo) <petri.savolainen@nsn.com> wrote: >> +void odp_coremask_from_str(const char *str, odp_coremask_t *mask) >> + ODP_NONNULL(1, 2); > > This is kind of ugly at API level. I was thinking that __attribute__((__nonnull_)) would work in (function) implementation. Is that possible? We can add easily a number of attributes into implementation to aid static checking, but API should be kept clean. > > Also according to this blog (from 2010), GCC has done "optimizations" based on the non-null tagging and e.g. removed if(ptr == NULL) checks from the function implementation. > > http://l.longi.li/blog/2010/04/19/gcc-s-attribute-nonnull-not-helpful-at-all/ If the state of warnings and code generation is still as described in this post, then attribute non-null is useless and counter-productive. > > > Could compiler be clever enough to spot non-null property from function implementation (from pointer check against NULL)? That code could be #ifdef'd there for static analysis / unit testing. Just because the code checks a parameter for non-null doesn't mean the compiler can understand this is an invalid parameter value. How can the code tell the *compiler* that based on some value checking, a parameter is invalid? There are no compiler directives (AFAIK) to invoke in such a situation and calling e.g. abort() is legal and could be the desired outcome from the caller's point of view. > > > -Petri > > > >> -----Original Message----- >> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- >> bounces@lists.linaro.org] On Behalf Of ext Mike Holmes >> Sent: Monday, January 12, 2015 11:44 PM >> To: lng-odp@lists.linaro.org >> Subject: [lng-odp] [PATCH 2/2] linux-genric: odp_core_mask add ODP_NONNULL >> >> Allow the complier to check for NULL args. >> >> Signed-off-by: Mike Holmes <mike.holmes@linaro.org> >> --- >> platform/linux-generic/include/api/odp_coremask.h | 18 +++++++++++------- >> 1 file changed, 11 insertions(+), 7 deletions(-) >> >> diff --git a/platform/linux-generic/include/api/odp_coremask.h >> b/platform/linux-generic/include/api/odp_coremask.h >> index c9331fd..b52f977 100644 >> --- a/platform/linux-generic/include/api/odp_coremask.h >> +++ b/platform/linux-generic/include/api/odp_coremask.h >> @@ -21,6 +21,7 @@ extern "C" { >> >> >> #include <odp_std_types.h> >> +#include <odp_hints.h> >> >> /** @addtogroup odp_scheduler >> * Core mask operations. >> @@ -51,7 +52,8 @@ typedef struct odp_coremask_t { >> * >> * @note Supports currently only core indexes upto 63 >> */ >> -void odp_coremask_from_str(const char *str, odp_coremask_t *mask); >> +void odp_coremask_from_str(const char *str, odp_coremask_t *mask) >> + ODP_NONNULL(1, 2); >> >> /** >> * Write core mask as a string of hexadecimal digits >> @@ -64,7 +66,8 @@ void odp_coremask_from_str(const char *str, >> odp_coremask_t *mask); >> * >> * @note Supports currently only core indexes upto 63 >> */ >> -void odp_coremask_to_str(char *str, int len, const odp_coremask_t *mask); >> +void odp_coremask_to_str(char *str, int len, const odp_coremask_t *mask) >> + ODP_NONNULL(1, 3); >> >> >> /** >> @@ -87,7 +90,8 @@ void odp_coremask_to_str(char *str, int len, const >> odp_coremask_t *mask); >> * >> * @note Supports currently only core indexes upto 63 >> */ >> -void odp_coremask_from_u64(const uint64_t *u64, int num, odp_coremask_t >> *mask); >> +void odp_coremask_from_u64(const uint64_t *u64, int num, odp_coremask_t >> *mask) >> + ODP_NONNULL(1, 3); >> >> /** >> * Clear entire mask >> @@ -103,14 +107,14 @@ static inline void odp_coremask_zero(odp_coremask_t >> *mask) >> * @param core Core number >> * @param mask add core number in core mask >> */ >> -void odp_coremask_set(int core, odp_coremask_t *mask); >> +void odp_coremask_set(int core, odp_coremask_t *mask) ODP_NONNULL(2); >> >> /** >> * Remove core from mask >> * @param core Core number >> * @param mask clear core number from core mask >> */ >> -void odp_coremask_clr(int core, odp_coremask_t *mask); >> +void odp_coremask_clr(int core, odp_coremask_t *mask) ODP_NONNULL(2); >> >> /** >> * Test if core is a member of mask >> @@ -118,14 +122,14 @@ void odp_coremask_clr(int core, odp_coremask_t >> *mask); >> * @param mask Core mask to check if core num set or not >> * @return non-zero if set otherwise 0 >> */ >> -int odp_coremask_isset(int core, const odp_coremask_t *mask); >> +int odp_coremask_isset(int core, const odp_coremask_t *mask) >> ODP_NONNULL(2); >> >> /** >> * Count number of cores in mask >> * @param mask Core mask >> * @return coremask count >> */ >> -int odp_coremask_count(const odp_coremask_t *mask); >> +int odp_coremask_count(const odp_coremask_t *mask) ODP_NONNULL(1); >> >> >> >> -- >> 2.1.0 >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
On 13 January 2015 at 04:29, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > On 13 January 2015 at 09:45, Savolainen, Petri (NSN - FI/Espoo) > <petri.savolainen@nsn.com> wrote: > >> +void odp_coremask_from_str(const char *str, odp_coremask_t *mask) > >> + ODP_NONNULL(1, 2); > > > > This is kind of ugly at API level. I was thinking that > __attribute__((__nonnull_)) would work in (function) implementation. Is > that possible? We can add easily a number of attributes into implementation > to aid static checking, but API should be kept clean. > > > > Also according to this blog (from 2010), GCC has done "optimizations" > based on the non-null tagging and e.g. removed if(ptr == NULL) checks from > the function implementation. > > > > > http://l.longi.li/blog/2010/04/19/gcc-s-attribute-nonnull-not-helpful-at-all/ > If the state of warnings and code generation is still as described in > this post, then attribute non-null is useless and counter-productive. > Interesting I need to read that in detail - posted in 2010 though so I wonder if it is still relevant > > > > > > Could compiler be clever enough to spot non-null property from function > implementation (from pointer check against NULL)? That code could be > #ifdef'd there for static analysis / unit testing. > Just because the code checks a parameter for non-null doesn't mean the > compiler can understand this is an invalid parameter value. How can > the code tell the *compiler* that based on some value checking, a > parameter is invalid? There are no compiler directives (AFAIK) to > invoke in such a situation and calling e.g. abort() is legal and could > be the desired outcome from the caller's point of view. > > > > > > -Petri > > > > > > > >> -----Original Message----- > >> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- > >> bounces@lists.linaro.org] On Behalf Of ext Mike Holmes > >> Sent: Monday, January 12, 2015 11:44 PM > >> To: lng-odp@lists.linaro.org > >> Subject: [lng-odp] [PATCH 2/2] linux-genric: odp_core_mask add > ODP_NONNULL > >> > >> Allow the complier to check for NULL args. > >> > >> Signed-off-by: Mike Holmes <mike.holmes@linaro.org> > >> --- > >> platform/linux-generic/include/api/odp_coremask.h | 18 > +++++++++++------- > >> 1 file changed, 11 insertions(+), 7 deletions(-) > >> > >> diff --git a/platform/linux-generic/include/api/odp_coremask.h > >> b/platform/linux-generic/include/api/odp_coremask.h > >> index c9331fd..b52f977 100644 > >> --- a/platform/linux-generic/include/api/odp_coremask.h > >> +++ b/platform/linux-generic/include/api/odp_coremask.h > >> @@ -21,6 +21,7 @@ extern "C" { > >> > >> > >> #include <odp_std_types.h> > >> +#include <odp_hints.h> > >> > >> /** @addtogroup odp_scheduler > >> * Core mask operations. > >> @@ -51,7 +52,8 @@ typedef struct odp_coremask_t { > >> * > >> * @note Supports currently only core indexes upto 63 > >> */ > >> -void odp_coremask_from_str(const char *str, odp_coremask_t *mask); > >> +void odp_coremask_from_str(const char *str, odp_coremask_t *mask) > >> + ODP_NONNULL(1, 2); > >> > >> /** > >> * Write core mask as a string of hexadecimal digits > >> @@ -64,7 +66,8 @@ void odp_coremask_from_str(const char *str, > >> odp_coremask_t *mask); > >> * > >> * @note Supports currently only core indexes upto 63 > >> */ > >> -void odp_coremask_to_str(char *str, int len, const odp_coremask_t > *mask); > >> +void odp_coremask_to_str(char *str, int len, const odp_coremask_t > *mask) > >> + ODP_NONNULL(1, 3); > >> > >> > >> /** > >> @@ -87,7 +90,8 @@ void odp_coremask_to_str(char *str, int len, const > >> odp_coremask_t *mask); > >> * > >> * @note Supports currently only core indexes upto 63 > >> */ > >> -void odp_coremask_from_u64(const uint64_t *u64, int num, odp_coremask_t > >> *mask); > >> +void odp_coremask_from_u64(const uint64_t *u64, int num, odp_coremask_t > >> *mask) > >> + ODP_NONNULL(1, 3); > >> > >> /** > >> * Clear entire mask > >> @@ -103,14 +107,14 @@ static inline void > odp_coremask_zero(odp_coremask_t > >> *mask) > >> * @param core Core number > >> * @param mask add core number in core mask > >> */ > >> -void odp_coremask_set(int core, odp_coremask_t *mask); > >> +void odp_coremask_set(int core, odp_coremask_t *mask) ODP_NONNULL(2); > >> > >> /** > >> * Remove core from mask > >> * @param core Core number > >> * @param mask clear core number from core mask > >> */ > >> -void odp_coremask_clr(int core, odp_coremask_t *mask); > >> +void odp_coremask_clr(int core, odp_coremask_t *mask) ODP_NONNULL(2); > >> > >> /** > >> * Test if core is a member of mask > >> @@ -118,14 +122,14 @@ void odp_coremask_clr(int core, odp_coremask_t > >> *mask); > >> * @param mask Core mask to check if core num set or not > >> * @return non-zero if set otherwise 0 > >> */ > >> -int odp_coremask_isset(int core, const odp_coremask_t *mask); > >> +int odp_coremask_isset(int core, const odp_coremask_t *mask) > >> ODP_NONNULL(2); > >> > >> /** > >> * Count number of cores in mask > >> * @param mask Core mask > >> * @return coremask count > >> */ > >> -int odp_coremask_count(const odp_coremask_t *mask); > >> +int odp_coremask_count(const odp_coremask_t *mask) ODP_NONNULL(1); > >> > >> > >> > >> -- > >> 2.1.0 > >> > >> > >> _______________________________________________ > >> lng-odp mailing list > >> lng-odp@lists.linaro.org > >> http://lists.linaro.org/mailman/listinfo/lng-odp > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > http://lists.linaro.org/mailman/listinfo/lng-odp >
diff --git a/platform/linux-generic/include/api/odp_coremask.h b/platform/linux-generic/include/api/odp_coremask.h index c9331fd..b52f977 100644 --- a/platform/linux-generic/include/api/odp_coremask.h +++ b/platform/linux-generic/include/api/odp_coremask.h @@ -21,6 +21,7 @@ extern "C" { #include <odp_std_types.h> +#include <odp_hints.h> /** @addtogroup odp_scheduler * Core mask operations. @@ -51,7 +52,8 @@ typedef struct odp_coremask_t { * * @note Supports currently only core indexes upto 63 */ -void odp_coremask_from_str(const char *str, odp_coremask_t *mask); +void odp_coremask_from_str(const char *str, odp_coremask_t *mask) + ODP_NONNULL(1, 2); /** * Write core mask as a string of hexadecimal digits @@ -64,7 +66,8 @@ void odp_coremask_from_str(const char *str, odp_coremask_t *mask); * * @note Supports currently only core indexes upto 63 */ -void odp_coremask_to_str(char *str, int len, const odp_coremask_t *mask); +void odp_coremask_to_str(char *str, int len, const odp_coremask_t *mask) + ODP_NONNULL(1, 3); /** @@ -87,7 +90,8 @@ void odp_coremask_to_str(char *str, int len, const odp_coremask_t *mask); * * @note Supports currently only core indexes upto 63 */ -void odp_coremask_from_u64(const uint64_t *u64, int num, odp_coremask_t *mask); +void odp_coremask_from_u64(const uint64_t *u64, int num, odp_coremask_t *mask) + ODP_NONNULL(1, 3); /** * Clear entire mask @@ -103,14 +107,14 @@ static inline void odp_coremask_zero(odp_coremask_t *mask) * @param core Core number * @param mask add core number in core mask */ -void odp_coremask_set(int core, odp_coremask_t *mask); +void odp_coremask_set(int core, odp_coremask_t *mask) ODP_NONNULL(2); /** * Remove core from mask * @param core Core number * @param mask clear core number from core mask */ -void odp_coremask_clr(int core, odp_coremask_t *mask); +void odp_coremask_clr(int core, odp_coremask_t *mask) ODP_NONNULL(2); /** * Test if core is a member of mask @@ -118,14 +122,14 @@ void odp_coremask_clr(int core, odp_coremask_t *mask); * @param mask Core mask to check if core num set or not * @return non-zero if set otherwise 0 */ -int odp_coremask_isset(int core, const odp_coremask_t *mask); +int odp_coremask_isset(int core, const odp_coremask_t *mask) ODP_NONNULL(2); /** * Count number of cores in mask * @param mask Core mask * @return coremask count */ -int odp_coremask_count(const odp_coremask_t *mask); +int odp_coremask_count(const odp_coremask_t *mask) ODP_NONNULL(1);
Allow the complier to check for NULL args. Signed-off-by: Mike Holmes <mike.holmes@linaro.org> --- platform/linux-generic/include/api/odp_coremask.h | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)