Fix compilation warnings in Lua and jemalloc dependencies #7785

yoonghm · 2020-09-11T00:53:43Z

Fix warnings mentioned in issue #7784.
The pull request is for lua module.

The argument `u` in the command `ar rcu` is ignore. Enable `LUA_USE_POSIX` to force the use of `mkstemp()` instead of `tmpname()`

Remove unused variable `c` in `f_parser()`

Removed misleadingly indented space in `luaL_loadfile()`

Removed misleadingly indented spaces within `addfield()`

oranagra · 2020-09-13T05:34:19Z

@yoonghm can you please tell me which OS / compiler are you using?
On mine (Ubuntu), i don't see the warning about mkstemp.
I'm not sure what are the consequences or side effect of the changes you made in the Makefile.
please look into it and list them here.

yoonghm · 2020-09-21T11:06:48Z

Hi, the warning will not show up unless you do a make distclean and then make.

oranagra · 2020-09-21T13:46:39Z

@yoonghm yeah, i know that.. i'm getting most of these warnings too, but not the one about mkstemp
please describe what would be the implications of the change you made in the makefile and list them here.

yoonghm · 2020-09-22T12:05:18Z

By default, Lua uses tmpnam except when POXIS is available, where it uses mkstemp. This solves the warning.

redis/deps/lua/src/luaconf.h

Lines 36 to 40 in b96c359

    
           #if defined(LUA_USE_LINUX) 
        
           #define LUA_USE_POSIX 
        
           #define LUA_USE_DLOPEN		/* needs an extra library: -ldl */ 
        
           #define LUA_USE_READLINE	/* needs some extra libraries */ 
        
           #endif

and also

redis/deps/lua/src/luaconf.h

Lines 636 to 660 in b96c359

    
           /* 
        
           @@ lua_tmpnam is the function that the OS library uses to create a 
        
           @* temporary name. 
        
           @@ LUA_TMPNAMBUFSIZE is the maximum size of a name created by lua_tmpnam. 
        
           ** CHANGE them if you have an alternative to tmpnam (which is considered 
        
           ** insecure) or if you want the original tmpnam anyway.  By default, Lua 
        
           ** uses tmpnam except when POSIX is available, where it uses mkstemp. 
        
           */ 
        
           #if defined(loslib_c) || defined(luaall_c) 
        
           #if defined(LUA_USE_MKSTEMP) 
        
           #include <unistd.h> 
        
           #define LUA_TMPNAMBUFSIZE	32 
        
           #define lua_tmpnam(b,e)	{ \ 
        
           	strcpy(b, "/tmp/lua_XXXXXX"); \ 
        
           	e = mkstemp(b); \ 
        
           	if (e != -1) close(e); \ 
        
           	e = (e == -1); } 
        
           #else 
        
           #define LUA_TMPNAMBUFSIZE	L_tmpnam 
        
           #define lua_tmpnam(b,e)		{ e = (tmpnam(b) == NULL); } 
        
           #endif 
        
           #endif

So, I changed ./deps/Makefile to define LUA_USE_POSIX.

LUA_CFLAGS+= -O2 -Wall -DLUA_ANSI -DENABLE_CJSON_GLOBAL -DREDIS_STATIC='' -DLUA_USE_POSIX $(CFLAGS)

The warning was from Ubuntu 2020.04

oranagra · 2020-09-22T12:27:42Z

but it also affects other things:

#define LUA_USE_ISATTY
#define LUA_USE_POPEN
#define LUA_USE_ULONGJMP

i want to know the full list of implications of this change.

btw, you still didn't tell me which OS / toolchain you're using.. i don't get this warning on my Ubuntu.

yoonghm · 2020-09-22T13:26:15Z

I have actually make test. You can try again. I did not see problem with my system so far using Redis 6.0.8.

I am using GCC on ubuntu running on x86.

cc (Ubuntu 9.3.0-10ubuntu2) 9.3.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE

The problem was actually mentioned a few times. See

Let me check if the define would cause any issue.

yoonghm · 2020-09-22T13:41:56Z

My investigation is that POSIX is meant for UNIX-Like operation system. If LUA_USE_POPEN is defined, it would use fflush() and pclose(). Otherwise for Windows platform, it would use _popen() and _pclose(). I have not came across a recent Redis client for Windows.

Developers are recommended to use WSL when running Redis within Windows 10.

oranagra · 2020-09-23T07:54:14Z

@yoonghm first there's also LUA_USE_ULONGJMP and LUA_USE_ISATTY.
Redis server (not client) is indeed not used on windows, but it should be very portable and is used on many OSes other than Linux (old and new).

My two concerns are:

make sure redis is still able to compile and run on all of them. i.e. that this change resolves a warning but doesn't prevent redis from building somewhere.
make sure that none of the changes, other than LUA_USE_MKSTEMP are affecting the Linux build (which is the main target for Redis in production environments) in some way (i.e. I was hoping we can prove that all the other changes don't impact the Linux build at all)

Can you please also explain the change to made to the ARFLAGS?
at the very least this comment needs to be updated:

# lua's Makefile defines AR="ar rcu", which is unusual, and makes it more
# challenging to cross-compile lua (and redis).  These defines make it easier
# to fit redis into cross-compilation environments, which typically set AR.

oranagra · 2020-09-23T08:24:31Z

btw, I see the lua makefile has many build targets, maybe we should somehow use these?

redis/deps/lua/src/Makefile

Line 84 in b96c359

aix:

yoonghm · 2020-09-23T15:45:42Z

Unix-like operating systems (Unix, Linux, MacOS) use ar to create static library from object files. On the other hand, Windows uses LIB to do similar thing.

I guess the original author of deps/Makefile had a vision that it could cross compile binaries for different operating system using a build host. Hence he created two variables AR and ARFLAGS which could be adopted for different operating systems. However, the author still used ar.

About the warning, binutil is now configured with --enable-deterministic-archives which is to create deterministic archives (or static library). Deterministic archives does not store timestamp, UID, GID, etc. Hence the u (replace newer object files) flag in rcu flags does not make sense.

I have no other operating systems with suitable compilers to test the build. Perhaps the deps/Makefile is modified to call deps/lua/Makefile instead.

LUA_USE_ULONGJMP and LUA_USE_ISATTY could not cause issue here, I believe.

oranagra · 2020-09-24T06:57:04Z

@yoonghm i know what ar is... i was hoping you'll add a short explanation of why removing that flag is ok, and doesn't have any bad side effects, so that i don't need to look into it myself.

i understand that currently it is ignored anyway, so dropping it won't actually change a thing, and even if it does, it just makes the archiving will take a bit longer, i doubt it's even measurable.

regarding LUA_USE_POSIX i tested your branch on our full CI, and everything passed:
https://github.com/oranagra/redis/runs/1155970307?check_suite_focus=true
however, i'm not sure what is the implication of all the other flags, saying that "could not cause issue here, I believe" is not enough for me. so the options are either to dive into it, or avoid making these changes.

in this case, i think that it doesn't worth the effort, all we need is to hide a warning of a bunch of dead code (Lua doesn't create files in Redis anyway), so i think we just want to explicitly set LUA_USE_MKSTEMP and not LUA_USE_POSIX. this way the only possible side effect is that Redis fails to build on some minor platform, and no risk for bugs or other side effects.

While working on your branch i did notice a few other warnings, with your permission, i'll push a few commits into this PR.

lower potential for side effects (all we want is solve a warning in dead code)

oranagra · 2020-09-24T07:15:03Z

looks like i'm unable to push commits directly to your branch, and github doesn't let me edit files which you didn't already modify, can you add this change (related to your other change):

--- a/deps/jemalloc/configure.ac
+++ b/deps/jemalloc/configure.ac
@@ -512,7 +512,7 @@ CTARGET='-o $@'
 LDTARGET='-o $@'
 TEST_LD_MODE=
 EXTRA_LDFLAGS=
-ARFLAGS='crus'
+ARFLAGS='crs'
 AROUT=' $@'
 CC_MM=1

i'll push other unrelated changes in a different PR

- The argument `u` in for `ar` is ignored (and generates warnings since `D` became the default. All it does is avoid updating unchanged objects (shouldn't have any impact on our build) - Enable `LUA_USE_MKSTEMP` to force the use of `mkstemp()` instead of `tmpname()` (which is dead code in redis anyway). - Remove unused variable `c` in `f_parser()` - Removed misleadingly indented space in `luaL_loadfile()` and ``addfield()` Co-authored-by: Oran Agra <oran@redislabs.com>

Upgraded to jemalloc 5.2.1 from 5.1.0. Cherry picked all relevant fixes (by diffing our 5.1.0 to upstream 5.10 and finding relevant commits). Details of what was done: [cherry-picked] fd7d51c 2021-05-03 Resolve nonsense static analysis warnings (Oran Agra) [cherry-picked] 448c435 2020-09-29 Fix compilation warnings in Lua and jemalloc dependencies (#7785) (YoongHM) [skipped - already in upstream] 9216b96 2020-09-21 Fix compilation warning in jemalloc's malloc_vsnprintf (#7789) (YoongHM) [cherry-picked] 88d71f4 2020-05-20 fix a rare active defrag edge case bug leading to stagnation (Oran Agra) [skipped - already in upstream] 2fec7d9 2019-05-30 Jemalloc: Avoid blocking on background thread lock for stats. [cherry-picked] 920158e 2018-07-11 Active defrag fixes for 32bit builds (again) (Oran Agra) [cherry-picked] e8099ca 2018-06-26 add defrag hint support into jemalloc 5 (Oran Agra) [re-done] 4e729fc 2018-05-24 Generate configure for Jemalloc. (antirez) Additionally had to do this: 7727cc2 2021-10-10 Fix defrag to support sharded bins in arena (added in v5.2.1) (Yoav Steinberg) When reviewing please look at all except the first commit which is just replacing 5.1.0 with 5.2.1 sources. Also I think we should merge this without squashing to preserve the changes we did to to jemalloc.

yoonghm added 4 commits September 11, 2020 08:43

Fix warning in /issues/7784

9b2b2ee

The argument `u` in the command `ar rcu` is ignore. Enable `LUA_USE_POSIX` to force the use of `mkstemp()` instead of `tmpname()`

Fix warning in /issues/7784

6315fc2

Remove unused variable `c` in `f_parser()`

Fix warning in /issues/7784

2ee950b

Removed misleadingly indented space in `luaL_loadfile()`

Fix warning in /issues/7784

39c2250

Removed misleadingly indented spaces within `addfield()`

oranagra changed the title ~~Redis#7784~~ Fix compilation warnings in Lua dependency Sep 13, 2020

oranagra linked an issue Sep 21, 2020 that may be closed by this pull request

Warnings found in make #7784

Closed

oranagra added the state:needs-investigation the problem or solution is not clear, some investigation is needed label Sep 23, 2020

oranagra removed the state:needs-investigation the problem or solution is not clear, some investigation is needed label Sep 24, 2020

use LUA_USE_MKSTEMP instead of LUA_USE_POSIX

c11df44

lower potential for side effects (all we want is solve a warning in dead code)

remove 'u' modifier from jemalloc's makefile

596be21

oranagra approved these changes Sep 29, 2020

View reviewed changes

oranagra requested a review from yossigo September 29, 2020 13:46

oranagra added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Sep 29, 2020

yossigo approved these changes Sep 29, 2020

View reviewed changes

oranagra changed the title ~~Fix compilation warnings in Lua dependency~~ Fix compilation warnings in Lua and jemalloc dependencies Sep 29, 2020

oranagra merged commit 448c435 into redis:unstable Sep 29, 2020

yoav-steinberg mentioned this pull request Oct 10, 2021

Upgrade jemalloc 5.2.1 #9623

Merged

Fix compilation warnings in Lua and jemalloc dependencies #7785

Fix compilation warnings in Lua and jemalloc dependencies #7785

Uh oh!

Conversation

yoonghm commented Sep 11, 2020

Uh oh!

oranagra commented Sep 13, 2020

Uh oh!

yoonghm commented Sep 21, 2020

Uh oh!

oranagra commented Sep 21, 2020

Uh oh!

yoonghm commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Sep 22, 2020

Uh oh!

yoonghm commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yoonghm commented Sep 22, 2020

Uh oh!

oranagra commented Sep 23, 2020

Uh oh!

oranagra commented Sep 23, 2020

Uh oh!

yoonghm commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Sep 24, 2020

Uh oh!

oranagra commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yoonghm commented Sep 22, 2020 •

edited

Loading

yoonghm commented Sep 22, 2020 •

edited

Loading

yoonghm commented Sep 23, 2020 •

edited

Loading

oranagra commented Sep 24, 2020 •

edited

Loading