* [Tarantool-patches] [PATCH] test: fix warning for old gcc at testgetmetrics.c
@ 2020-10-17 11:44 Sergey Kaplun
2020-10-20 11:06 ` Igor Munkin
0 siblings, 1 reply; 5+ messages in thread
From: Sergey Kaplun @ 2020-10-17 11:44 UTC (permalink / raw)
To: Igor Munkin, Sergey Ostanevich; +Cc: tarantool-patches
This patch fixes the regression introduced in scope of
5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua
API for platform metrics').
Old gcc compiler (gcc-4.8.5-39) throws warnings like:
"error: comparison is always true due to limited range of data type
[-Werror=type-limits]", when you cast syze_t value to ssyze_t type and
compare its to >= 0. This leads to failing compilation with -Werror
flag.
Taking into account that this base test check only API format, not
values by themself, it will be enough to use (void) construction here.
Follows up tarantool/tarantool#5187
---
Bug was reported by Sasha Tikhonov. He faced it up when test his own
branch (avtikhon/gh-4941-gcc-old) with enabled default gcc at Centos 7.
There was failure like [1]:
===================================================================
/build/usr/src/debug/tarantool-2.6.0.208/third_party/luajit/test/misclib-getmetrics-capi/testgetmetrics.c:41:9: error: comparison is always true due to limited range of data type [-Werror=type-limits]
assert((ssize_t)metrics.jit_trace_num >= 0);
^
===================================================================
Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5187-old-gcc-warning
Branch for test CI was rebased on top of avtikhon/gh-4941-gcc-old which
enables CI-checks with default gcc on CentOS 7
CI is still red but now there are tons of warnings like [2]:
===================================================================
/usr/include/features.h:330:4: error: #warning _FORTIFY_SOURCE requires compiling with optimization (-O) [-Werror=cpp]
# warning _FORTIFY_SOURCE requires compiling with optimization (-O)
===================================================================
or replication|txn_manager (see also [3]) tests failures not related to
LuaJIT.
Side note: This is old pipeline, but I changed only comment format
before repushed branch.
CI: https://gitlab.com/tarantool/tarantool/-/pipelines/203899226
[1]: https://gitlab.com/tarantool/tarantool/-/jobs/795394423#L1999
[2]: https://gitlab.com/tarantool/tarantool/-/jobs/796433283#L1637
[3]: https://github.com/tarantool/tarantool/issues/5423
test/misclib-getmetrics-capi/testgetmetrics.c | 51 +++++++++----------
1 file changed, 24 insertions(+), 27 deletions(-)
diff --git a/test/misclib-getmetrics-capi/testgetmetrics.c b/test/misclib-getmetrics-capi/testgetmetrics.c
index 3b6f599..fc19680 100644
--- a/test/misclib-getmetrics-capi/testgetmetrics.c
+++ b/test/misclib-getmetrics-capi/testgetmetrics.c
@@ -12,33 +12,30 @@ static int base(lua_State *L)
struct luam_Metrics metrics;
luaM_metrics(L, &metrics);
- /*
- * Just check API.
- * (ssize_t) cast need for not always-true unsigned >= 0 check.
- */
- assert((ssize_t)metrics.strhash_hit >= 0);
- assert((ssize_t)metrics.strhash_miss >= 0);
-
- assert((ssize_t)metrics.gc_strnum >= 0);
- assert((ssize_t)metrics.gc_tabnum >= 0);
- assert((ssize_t)metrics.gc_udatanum >= 0);
- assert((ssize_t)metrics.gc_cdatanum >= 0);
-
- assert((ssize_t)metrics.gc_total >= 0);
- assert((ssize_t)metrics.gc_freed >= 0);
- assert((ssize_t)metrics.gc_allocated >= 0);
-
- assert((ssize_t)metrics.gc_steps_pause >= 0);
- assert((ssize_t)metrics.gc_steps_propagate >= 0);
- assert((ssize_t)metrics.gc_steps_atomic >= 0);
- assert((ssize_t)metrics.gc_steps_sweepstring >= 0);
- assert((ssize_t)metrics.gc_steps_sweep >= 0);
- assert((ssize_t)metrics.gc_steps_finalize >= 0);
-
- assert((ssize_t)metrics.jit_snap_restore >= 0);
- assert((ssize_t)metrics.jit_trace_abort >= 0);
- assert((ssize_t)metrics.jit_mcode_size >= 0);
- assert((ssize_t)metrics.jit_trace_num >= 0);
+ /* Just check API, not values by itself. */
+ (void)metrics.strhash_hit;
+ (void)metrics.strhash_miss;
+
+ (void)metrics.gc_strnum;
+ (void)metrics.gc_tabnum;
+ (void)metrics.gc_udatanum;
+ (void)metrics.gc_cdatanum;
+
+ (void)metrics.gc_total;
+ (void)metrics.gc_freed;
+ (void)metrics.gc_allocated;
+
+ (void)metrics.gc_steps_pause;
+ (void)metrics.gc_steps_propagate;
+ (void)metrics.gc_steps_atomic;
+ (void)metrics.gc_steps_sweepstring;
+ (void)metrics.gc_steps_sweep;
+ (void)metrics.gc_steps_finalize;
+
+ (void)metrics.jit_snap_restore;
+ (void)metrics.jit_trace_abort;
+ (void)metrics.jit_mcode_size;
+ (void)metrics.jit_trace_num;
lua_pushboolean(L, 1);
return 1;
--
2.28.0
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH] test: fix warning for old gcc at testgetmetrics.c
2020-10-17 11:44 [Tarantool-patches] [PATCH] test: fix warning for old gcc at testgetmetrics.c Sergey Kaplun
@ 2020-10-20 11:06 ` Igor Munkin
2020-10-20 12:44 ` Sergey Ostanevich
0 siblings, 1 reply; 5+ messages in thread
From: Igor Munkin @ 2020-10-20 11:06 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
Sergey,
Thanks for the patch! The patch LGTM, but there are several nits below,
please consider them.
On 17.10.20, Sergey Kaplun wrote:
> This patch fixes the regression introduced in scope of
> 5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua
> API for platform metrics').
>
> Old gcc compiler (gcc-4.8.5-39) throws warnings like:
> "error: comparison is always true due to limited range of data type
> [-Werror=type-limits]", when you cast syze_t value to ssyze_t type and
Typo: s/syze_t/size_t/g.
> compare its to >= 0. This leads to failing compilation with -Werror
> flag.
>
> Taking into account that this base test check only API format, not
Typo: s/check/checks/.
> values by themself, it will be enough to use (void) construction here.
>
> Follows up tarantool/tarantool#5187
> ---
> Bug was reported by Sasha Tikhonov. He faced it up when test his own
> branch (avtikhon/gh-4941-gcc-old) with enabled default gcc at Centos 7.
> There was failure like [1]:
> ===================================================================
> /build/usr/src/debug/tarantool-2.6.0.208/third_party/luajit/test/misclib-getmetrics-capi/testgetmetrics.c:41:9: error: comparison is always true due to limited range of data type [-Werror=type-limits]
> assert((ssize_t)metrics.jit_trace_num >= 0);
> ^
> ===================================================================
>
> Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5187-old-gcc-warning
>
> Branch for test CI was rebased on top of avtikhon/gh-4941-gcc-old which
> enables CI-checks with default gcc on CentOS 7
>
> CI is still red but now there are tons of warnings like [2]:
> ===================================================================
> /usr/include/features.h:330:4: error: #warning _FORTIFY_SOURCE requires compiling with optimization (-O) [-Werror=cpp]
> # warning _FORTIFY_SOURCE requires compiling with optimization (-O)
Side note: this is faced here, this is also faced while package
building. I hope this will tire someone and one will finally fix this
crap in our CMake recipes.
> ===================================================================
> or replication|txn_manager (see also [3]) tests failures not related to
> LuaJIT.
>
> Side note: This is old pipeline, but I changed only comment format
> before repushed branch.
>
> CI: https://gitlab.com/tarantool/tarantool/-/pipelines/203899226
>
> [1]: https://gitlab.com/tarantool/tarantool/-/jobs/795394423#L1999
> [2]: https://gitlab.com/tarantool/tarantool/-/jobs/796433283#L1637
> [3]: https://github.com/tarantool/tarantool/issues/5423
>
> test/misclib-getmetrics-capi/testgetmetrics.c | 51 +++++++++----------
> 1 file changed, 24 insertions(+), 27 deletions(-)
>
> diff --git a/test/misclib-getmetrics-capi/testgetmetrics.c b/test/misclib-getmetrics-capi/testgetmetrics.c
> index 3b6f599..fc19680 100644
> --- a/test/misclib-getmetrics-capi/testgetmetrics.c
> +++ b/test/misclib-getmetrics-capi/testgetmetrics.c
> @@ -12,33 +12,30 @@ static int base(lua_State *L)
> struct luam_Metrics metrics;
> luaM_metrics(L, &metrics);
>
> - /*
> - * Just check API.
> - * (ssize_t) cast need for not always-true unsigned >= 0 check.
> - */
> - assert((ssize_t)metrics.strhash_hit >= 0);
> - assert((ssize_t)metrics.strhash_miss >= 0);
> -
> - assert((ssize_t)metrics.gc_strnum >= 0);
> - assert((ssize_t)metrics.gc_tabnum >= 0);
> - assert((ssize_t)metrics.gc_udatanum >= 0);
> - assert((ssize_t)metrics.gc_cdatanum >= 0);
> -
> - assert((ssize_t)metrics.gc_total >= 0);
> - assert((ssize_t)metrics.gc_freed >= 0);
> - assert((ssize_t)metrics.gc_allocated >= 0);
> -
> - assert((ssize_t)metrics.gc_steps_pause >= 0);
> - assert((ssize_t)metrics.gc_steps_propagate >= 0);
> - assert((ssize_t)metrics.gc_steps_atomic >= 0);
> - assert((ssize_t)metrics.gc_steps_sweepstring >= 0);
> - assert((ssize_t)metrics.gc_steps_sweep >= 0);
> - assert((ssize_t)metrics.gc_steps_finalize >= 0);
> -
> - assert((ssize_t)metrics.jit_snap_restore >= 0);
> - assert((ssize_t)metrics.jit_trace_abort >= 0);
> - assert((ssize_t)metrics.jit_mcode_size >= 0);
> - assert((ssize_t)metrics.jit_trace_num >= 0);
> + /* Just check API, not values by itself. */
Minor: It looks like only structure fields check (i.e. interface,
contract), but not the values they contain (i.e. API), so I propose to
adjust the wording.
> + (void)metrics.strhash_hit;
> + (void)metrics.strhash_miss;
> +
> + (void)metrics.gc_strnum;
> + (void)metrics.gc_tabnum;
> + (void)metrics.gc_udatanum;
> + (void)metrics.gc_cdatanum;
> +
> + (void)metrics.gc_total;
> + (void)metrics.gc_freed;
> + (void)metrics.gc_allocated;
> +
> + (void)metrics.gc_steps_pause;
> + (void)metrics.gc_steps_propagate;
> + (void)metrics.gc_steps_atomic;
> + (void)metrics.gc_steps_sweepstring;
> + (void)metrics.gc_steps_sweep;
> + (void)metrics.gc_steps_finalize;
> +
> + (void)metrics.jit_snap_restore;
> + (void)metrics.jit_trace_abort;
> + (void)metrics.jit_mcode_size;
> + (void)metrics.jit_trace_num;
>
> lua_pushboolean(L, 1);
> return 1;
> --
> 2.28.0
>
--
Best regards,
IM
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH] test: fix warning for old gcc at testgetmetrics.c
2020-10-20 11:06 ` Igor Munkin
@ 2020-10-20 12:44 ` Sergey Ostanevich
2020-10-20 12:59 ` Sergey Kaplun
0 siblings, 1 reply; 5+ messages in thread
From: Sergey Ostanevich @ 2020-10-20 12:44 UTC (permalink / raw)
To: Igor Munkin; +Cc: tarantool-patches
Hi!
The patch is LGTM, wording in the commit message still needs updates
(the latest branch log below).
Sergos
> commit 7a4c686e61bb70defe56e4e823ae09ce98c8dc04 (HEAD -> skaplun/gh-5187-old-gcc-warning, origin/skaplun/gh-5187-old-gcc-warning)
> Author: Sergey Kaplun <skaplun@tarantool.org>
> Date: Sat Oct 17 09:13:07 2020 +0300
>
> test: fix warning for old gcc at testgetmetrics.c
>
> This patch fixes the regression introduced in scope of
> 5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua
> API for platform metrics').
>
> Old gcc compiler (gcc-4.8.5-39) throws warnings like:
> "error: comparison is always true due to limited range of data type
> [-Werror=type-limits]", when you cast syze_t value to ssyze_t type and
size instead of syze
> compare its to >= 0. This leads to failing compilation with -Werror
compare it, not its
> flag.
>
> Taking into account that this base test check only API format, not
> values by themself, it will be enough to use (void) construction here.
by themself -> themselves
>
> Follows up tarantool/tarantool#5187
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH] test: fix warning for old gcc at testgetmetrics.c
2020-10-20 12:44 ` Sergey Ostanevich
@ 2020-10-20 12:59 ` Sergey Kaplun
2020-10-20 15:17 ` Alexander V. Tikhonov
0 siblings, 1 reply; 5+ messages in thread
From: Sergey Kaplun @ 2020-10-20 12:59 UTC (permalink / raw)
To: Sergey Ostanevich; +Cc: tarantool-patches
Sergos, Igor,
Thanks for the review!
On 20.10.20, Sergey Ostanevich wrote:
> Hi!
>
> The patch is LGTM, wording in the commit message still needs updates
> (the latest branch log below).
>
> Sergos
>
> > commit 7a4c686e61bb70defe56e4e823ae09ce98c8dc04 (HEAD -> skaplun/gh-5187-old-gcc-warning, origin/skaplun/gh-5187-old-gcc-warning)
> > Author: Sergey Kaplun <skaplun@tarantool.org>
> > Date: Sat Oct 17 09:13:07 2020 +0300
> >
> > test: fix warning for old gcc at testgetmetrics.c
> >
> > This patch fixes the regression introduced in scope of
> > 5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua
> > API for platform metrics').
> >
> > Old gcc compiler (gcc-4.8.5-39) throws warnings like:
> > "error: comparison is always true due to limited range of data type
> > [-Werror=type-limits]", when you cast syze_t value to ssyze_t type and
>
> size instead of syze
Thanks, fixed!
>
> > compare its to >= 0. This leads to failing compilation with -Werror
>
> compare it, not its
Thanks, fixed.
>
> > flag.
> >
> > Taking into account that this base test check only API format, not
> > values by themself, it will be enough to use (void) construction here.
>
> by themself -> themselves
Thanks, fixed!
>
> >
> > Follows up tarantool/tarantool#5187
> >
>
So, new commit message is:
===================================================================
test: fix warning for old gcc at testgetmetrics.c
This patch fixes the regression introduced in scope of
5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua
API for platform metrics').
Old gcc compiler (gcc-4.8.5-39) throws warnings like:
"error: comparison is always true due to limited range of data type
[-Werror=type-limits]", when you cast size_t value to ssize_t type and
compare it to >= 0. This leads to failing compilation with -Werror
flag.
Taking into account that this base test checks only API format, not
values by themselves, it will be enough to use (void) construction here.
Follows up tarantool/tarantool#5187
===================================================================
Also updated comment. See iterative patch below. Branch is force
pushed.
===================================================================
diff --git a/test/misclib-getmetrics-capi/testgetmetrics.c b/test/misclib-getmetrics-capi/testgetmetrics.c
index fc19680..7fd3eef 100644
--- a/test/misclib-getmetrics-capi/testgetmetrics.c
+++ b/test/misclib-getmetrics-capi/testgetmetrics.c
@@ -12,7 +12,7 @@ static int base(lua_State *L)
struct luam_Metrics metrics;
luaM_metrics(L, &metrics);
- /* Just check API, not values by itself. */
+ /* Just check structure format, not values that fields contain. */
(void)metrics.strhash_hit;
(void)metrics.strhash_miss;
===================================================================
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH] test: fix warning for old gcc at testgetmetrics.c
2020-10-20 12:59 ` Sergey Kaplun
@ 2020-10-20 15:17 ` Alexander V. Tikhonov
0 siblings, 0 replies; 5+ messages in thread
From: Alexander V. Tikhonov @ 2020-10-20 15:17 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
Hi Sergey, thanks for the patch. I've checked your fix and it really
helped with building using gcc-4.8.5 with Werror flag [1]. Also no new
degradations occurred [2]. Patch LGTM.
[1] - https://gitlab.com/tarantool/tarantool/-/jobs/801762908
[2] - https://gitlab.com/tarantool/tarantool/-/pipelines/205243134
On Tue, Oct 20, 2020 at 03:59:10PM +0300, Sergey Kaplun wrote:
> Sergos, Igor,
>
> Thanks for the review!
>
> On 20.10.20, Sergey Ostanevich wrote:
> > Hi!
> >
> > The patch is LGTM, wording in the commit message still needs updates
> > (the latest branch log below).
> >
> > Sergos
> >
> > > commit 7a4c686e61bb70defe56e4e823ae09ce98c8dc04 (HEAD -> skaplun/gh-5187-old-gcc-warning, origin/skaplun/gh-5187-old-gcc-warning)
> > > Author: Sergey Kaplun <skaplun@tarantool.org>
> > > Date: Sat Oct 17 09:13:07 2020 +0300
> > >
> > > test: fix warning for old gcc at testgetmetrics.c
> > >
> > > This patch fixes the regression introduced in scope of
> > > 5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua
> > > API for platform metrics').
> > >
> > > Old gcc compiler (gcc-4.8.5-39) throws warnings like:
> > > "error: comparison is always true due to limited range of data type
> > > [-Werror=type-limits]", when you cast syze_t value to ssyze_t type and
> >
> > size instead of syze
>
> Thanks, fixed!
>
> >
> > > compare its to >= 0. This leads to failing compilation with -Werror
> >
> > compare it, not its
>
> Thanks, fixed.
>
> >
> > > flag.
> > >
> > > Taking into account that this base test check only API format, not
> > > values by themself, it will be enough to use (void) construction here.
> >
> > by themself -> themselves
>
> Thanks, fixed!
>
> >
> > >
> > > Follows up tarantool/tarantool#5187
> > >
> >
>
> So, new commit message is:
>
> ===================================================================
> test: fix warning for old gcc at testgetmetrics.c
>
> This patch fixes the regression introduced in scope of
> 5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua
> API for platform metrics').
>
> Old gcc compiler (gcc-4.8.5-39) throws warnings like:
> "error: comparison is always true due to limited range of data type
> [-Werror=type-limits]", when you cast size_t value to ssize_t type and
> compare it to >= 0. This leads to failing compilation with -Werror
> flag.
>
> Taking into account that this base test checks only API format, not
> values by themselves, it will be enough to use (void) construction here.
>
> Follows up tarantool/tarantool#5187
> ===================================================================
>
> Also updated comment. See iterative patch below. Branch is force
> pushed.
>
> ===================================================================
> diff --git a/test/misclib-getmetrics-capi/testgetmetrics.c b/test/misclib-getmetrics-capi/testgetmetrics.c
> index fc19680..7fd3eef 100644
> --- a/test/misclib-getmetrics-capi/testgetmetrics.c
> +++ b/test/misclib-getmetrics-capi/testgetmetrics.c
> @@ -12,7 +12,7 @@ static int base(lua_State *L)
> struct luam_Metrics metrics;
> luaM_metrics(L, &metrics);
>
> - /* Just check API, not values by itself. */
> + /* Just check structure format, not values that fields contain. */
> (void)metrics.strhash_hit;
> (void)metrics.strhash_miss;
>
> ===================================================================
>
> --
> Best regards,
> Sergey Kaplun
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-10-20 15:17 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-17 11:44 [Tarantool-patches] [PATCH] test: fix warning for old gcc at testgetmetrics.c Sergey Kaplun
2020-10-20 11:06 ` Igor Munkin
2020-10-20 12:44 ` Sergey Ostanevich
2020-10-20 12:59 ` Sergey Kaplun
2020-10-20 15:17 ` Alexander V. Tikhonov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox