Skip to content

Fix clang warnings #103

Closed
wants to merge 12 commits into from

6 participants

@proxyles

No description provided.

nox added some commits Oct 14, 2013
@nox nox Remove uninitialized use of new_crr in erl_alloc_util
When the offending code is reached, new_crr is either uninitalized or
have been set to NULL. This patch removes the following warning:

beam/erl_alloc_util.c:3510:6: warning: variable 'new_crr' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
        if (!(flags & CFLG_FORCE_MSEG)) {
            ^~~~~~~~~~~~~~~~~~~~~~~~~~
beam/erl_alloc_util.c:3567:23: note: uninitialized use occurs here
        DEBUG_SAVE_ALIGNMENT(new_crr);
                             ^~~~~~~
beam/erl_alloc_util.c:674:51: note: expanded from macro 'DEBUG_SAVE_ALIGNMENT'
    UWord algnmnt__ = sizeof(Unit_t) - (((UWord) (C)) % sizeof(Unit_t));\
                                                  ^
beam/erl_alloc_util.c:3510:2: note: remove the 'if' if its condition is always true
        if (!(flags & CFLG_FORCE_MSEG)) {
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
beam/erl_alloc_util.c:3438:23: note: initialize the variable 'new_crr' to silence this warning
    Carrier_t *new_crr, *old_crr;
                      ^
                       = NULL
764139c
@nox nox Properly mark erl_assert_error as non-returning bb4d0cd
@nox nox Fix erts_check_off_heap2 assertion
The test needs to be false to fail, not true. This was noticed with the
following warning, where marking erl_assert_error as non-returning didn't
help:

beam/erl_gc.c:2772:2: warning: variable 'refc' is used uninitialized whenever switch default is taken [-Wsometimes-uninitialized]
        default:
        ^~~~~~~
beam/erl_gc.c:2775:26: note: uninitialized use occurs here
        ERTS_CHK_OFFHEAP_ASSERT(refc >= 1);
                                ^~~~
beam/erl_gc.c:2738:11: note: expanded from macro 'ERTS_CHK_OFFHEAP_ASSERT'
    if (!(EXP))                                         \
          ^
beam/erl_gc.c:2759:18: note: initialize the variable 'refc' to silence this warning
        erts_aint_t refc;
                        ^
                         = 0
fee950e
@nox nox Silence a warning in erl_poll about an empty loop body
The warning is:

sys/common/erl_poll.c:2513:72: warning: for loop has empty body [-Wempty-body]
        for (prev_ps = pollsets; ps != prev_ps->next; prev_ps = prev_ps->next);
fad8c6e
@nox nox Silence warnings about unused return values in ic
The warnings are:

oe_ei_encode_string.c:26:3: warning: expression result unused [-Wunused-value]
  (int) ei_encode_string(0,&size,p);
  ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~

oe_ei_encode_port.c:26:3: warning: expression result unused [-Wunused-value]
  (int) ei_encode_port(NULL, &size, p);
  ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

oe_ei_encode_atom.c:26:3: warning: expression result unused [-Wunused-value]
  (int) ei_encode_atom(0,&size,p);
  ^     ~~~~~~~~~~~~~~~~~~~~~~~~~

oe_ei_encode_pid.c:26::3: warning: expression result unused [-Wunused-value]
  (int) ei_encode_pid(NULL, &size, p);
  ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

oe_ei_encode_ref.c:26:3: warning: expression result unused [-Wunused-value]
  (int) ei_encode_ref(NULL, &size, p);
  ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

oe_ei_encode_term.c:26:3: warning: expression result unused [-Wunused-value]
  (int) ei_encode_term(NULL, &size, t);
  ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
f0b4604
@nox nox Silence warnings about socklen_t pointers sign in erl_memory
erl_memory.c:947:7: warning: passing 'int *' to parameter of type 'socklen_t *' (aka 'unsigned int *') converts between pointers to integer types with different sign [-Wpointer-sign]
                    &saddr_size) != 0)
                    ^~~~~~~~~~~
/usr/include/sys/socket.h:616:74: note: passing argument to parameter here
int     getsockname(int, struct sockaddr * __restrict, socklen_t * __restrict)
                                                                             ^

erl_memory.c:2613:57: warning: passing 'int *' to parameter of type 'socklen_t *' (aka 'unsigned int *') converts between pointers to integer types with different sign [-Wpointer-sign]
    sock = accept(lsock, (struct sockaddr *) &oth_addr, &oth_addr_len);
                                                        ^~~~~~~~~~~~~
/usr/include/sys/socket.h:610:69: note: passing argument to parameter here
int     accept(int, struct sockaddr * __restrict, socklen_t * __restrict)
                                                                        ^
b32d7fb
@nox
nox commented Oct 15, 2013

I just amended some commits to fix some typos, please refetch.

@OTP-Maintainer

Unable to build otp: xe_events.o: No such file or directory
g++: error: x86_64-unknown-linux-gnu/wxe_init.o: No such file or directory
make[3]: *** [../priv/x86_64-unknown-linux-gnu/wxe_driver.so] Error 1
make[3]: Leaving directory /ldisk/jenkins/workspace/pullrequests/otp/lib/wx/c_src'
make[2]: *** [opt] Error 2
make[2]: Leaving directory
/ldisk/jenkins/workspace/pullrequests/otp/lib/wx'
make[1]: *** [opt] Error 2
make[1]: Leaving directory `/ldisk/jenkins/workspace/pullrequests/otp/lib'
make: *** [libs] Error 2

@nox
nox commented Oct 16, 2013

Forgot to include output.mk in the wx Makefile. This is fixed now.

Btw @OTP-Maintainer you should put the build output in a code block.

@gustehn
gustehn commented Oct 16, 2013

@nox I should fix that, thanks for pointing it out. Also, maybe you should try to build your patches before submitting them ? ;)

@nox
nox commented Oct 16, 2013

@gustehn Tried it, but I messed up at one point and updated Makefile instead of Makefile.in :)

@nox
nox commented Oct 16, 2013

Also maybe you guys should properly use silent rules when tweaking Makefiles so that I don't have to check that you did not forget to support them :D

@OTP-Maintainer

Unable to build otp: xe_events.o: No such file or directory
g++: error: x86_64-unknown-linux-gnu/wxe_init.o: No such file or directory
make[3]: *** [../priv/x86_64-unknown-linux-gnu/wxe_driver.so] Error 1
make[3]: Leaving directory /ldisk/jenkins/workspace/pullrequests/otp/lib/wx/c_src'
make[2]: *** [opt] Error 2
make[2]: Leaving directory
/ldisk/jenkins/workspace/pullrequests/otp/lib/wx'
make[1]: *** [opt] Error 2
make[1]: Leaving directory `/ldisk/jenkins/workspace/pullrequests/otp/lib'
make: *** [libs] Error 2

@nox
nox commented Oct 17, 2013

And this time a badly set ERL_TOP is to blame. Cloned from scratch just to be sure that it works.

@gustehn: Btw there is something weird in the pasted output: there is no xe_events.o file built by Erlang/OTP, the first character seems stripped out.

@OTP-Maintainer

Unable to build otp: iameter/src'
make[3]: Entering directory /ldisk/jenkins/workspace/pullrequests/otp/lib/diameter/doc/src'
Makefile:144: *** commands commence before first target. Stop.
make[3]: Leaving directory
/ldisk/jenkins/workspace/pullrequests/otp/lib/diameter/doc/src'
make[2]: *** [opt] Error 2
make[2]: Leaving directory /ldisk/jenkins/workspace/pullrequests/otp/lib/diameter'
make[1]: *** [opt] Error 2
make[1]: Leaving directory
/ldisk/jenkins/workspace/pullrequests/otp/lib'
make: *** [libs] Error 2

@fenollp
fenollp commented Oct 17, 2013

Are you sure you know what you are doing here?
nox@69c5861
Your code may not be portable as http://stackoverflow.com/a/75238/1418165 suggests.
Also you replaced every usgnd_int_8 * by a char *, I would have replaced those with uint8.

Am I off?

@nox
nox commented Oct 17, 2013

@fenollp These should contain only ASCII characters so the signedness either shouldn't be a problem or we have one already because we pass those to printf functions. Hence the warnings. Most commits in that branch change the code to silence warnings to spark the discussion about why these warnings happen in the first place.

@fenollp
fenollp commented Oct 18, 2013

Ok good. On a side note there are a couple explicit casts that seem superfluous eg: https://github.com/nox/otp/blob/69c5861feca44ac261bd63a2d053c87ba5ff241c/lib/tools/c_src/erl_memory.c#L922
Was clang really complaining about those or were you a little fast at silencing those warnings?

@nox
nox commented Oct 18, 2013

Mmh I casted that one prior to changing em_area.ptr's type, thus making it useless. Will amend.

nox added some commits Oct 14, 2013
@nox nox Silence warnings about different buffer types in erl_memory
erl_memory.c:861:19: warning: passing 'usgnd_int_8 *' (aka 'unsigned char *') to parameter of type 'char *' converts between pointers to integer types with different sign [-Wpointer-sign]
    *p += sprintf(*p, "%*" USGND_INT_MAX_FSTR " ", fw, mi->size);
                  ^~
/usr/include/secure/_stdio.h:49:28: note: expanded from macro 'sprintf'
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
                           ^
erl_memory.c:863:16: warning: passing 'usgnd_int_8 *' (aka 'unsigned char *') to parameter of type 'char *' converts between pointers to integer types with different sign [-Wpointer-sign]
        *p += sprintf(*p,
                      ^~
/usr/include/secure/_stdio.h:49:28: note: expanded from macro 'sprintf'
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
                           ^
erl_memory.c:869:16: warning: passing 'usgnd_int_8 *' (aka 'unsigned char *') to parameter of type 'char *' converts between pointers to integer types with different sign [-Wpointer-sign]
        *p += sprintf(*p, "%*" USGND_INT_MAX_FSTR " ", fw, mi->no);
                      ^~
/usr/include/secure/_stdio.h:49:28: note: expanded from macro 'sprintf'
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
                           ^
erl_memory.c:871:20: warning: passing 'usgnd_int_8 *' (aka 'unsigned char *') to parameter of type 'char *' converts between pointers to integer types with different sign [-Wpointer-sign]
            *p += sprintf(*p,
                          ^~
/usr/include/secure/_stdio.h:49:28: note: expanded from macro 'sprintf'
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
                           ^
erl_memory.c:878:16: warning: passing 'usgnd_int_8 *' (aka 'unsigned char *') to parameter of type 'char *' converts between pointers to integer types with different sign [-Wpointer-sign]
        *p += sprintf(*p,
                      ^~
/usr/include/secure/_stdio.h:49:28: note: expanded from macro 'sprintf'
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
                           ^
erl_memory.c:900:19: warning: passing 'usgnd_int_8 *' (aka 'unsigned char *') to parameter of type 'char *' converts between pointers to integer types with different sign [-Wpointer-sign]
    *p += sprintf(*p, "%*" USGND_INT_MAX_FSTR " ", fw, mi->max_ever_size);
                  ^~
/usr/include/secure/_stdio.h:49:28: note: expanded from macro 'sprintf'
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
                           ^
erl_memory.c:902:16: warning: passing 'usgnd_int_8 *' (aka 'unsigned char *') to parameter of type 'char *' converts between pointers to integer types with different sign [-Wpointer-sign]
        *p += sprintf(*p, "%*s %*s ", fw, "", fw, "");
                      ^~
/usr/include/secure/_stdio.h:49:28: note: expanded from macro 'sprintf'
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
                           ^
erl_memory.c:904:16: warning: passing 'usgnd_int_8 *' (aka 'unsigned char *') to parameter of type 'char *' converts between pointers to integer types with different sign [-Wpointer-sign]
        *p += sprintf(*p, "%*" USGND_INT_MAX_FSTR " ", fw, mi->max_ever_no);
                      ^~
/usr/include/secure/_stdio.h:49:28: note: expanded from macro 'sprintf'
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
                           ^
erl_memory.c:906:20: warning: passing 'usgnd_int_8 *' (aka 'unsigned char *') to parameter of type 'char *' converts between pointers to integer types with different sign [-Wpointer-sign]
            *p += sprintf(*p, "%*s %*s ", fw, "", fw, "");
                          ^~
/usr/include/secure/_stdio.h:49:28: note: expanded from macro 'sprintf'
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
                           ^
erl_memory.c:909:16: warning: passing 'usgnd_int_8 *' (aka 'unsigned char *') to parameter of type 'char *' converts between pointers to integer types with different sign [-Wpointer-sign]
        *p += sprintf(*p, "%*s %*s %*s ", fw, "", fw, "", fw, "");
                      ^~
/usr/include/secure/_stdio.h:49:28: note: expanded from macro 'sprintf'
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
                           ^
erl_memory.c:986:25: warning: passing 'usgnd_int_8 *' (aka 'unsigned char *') to parameter of type 'char *' converts between pointers to integer types with different sign [-Wpointer-sign]
    area.size = sprintf(area.ptr, format, carg);
                        ^~~~~~~~
/usr/include/secure/_stdio.h:49:28: note: expanded from macro 'sprintf'
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
                           ^
erl_memory.c:1103:25: warning: passing 'usgnd_int_8 *' (aka 'unsigned char *') to parameter of type 'char *' converts between pointers to integer types with different sign [-Wpointer-sign]
    area.size = sprintf(area.ptr,
                        ^~~~~~~~
/usr/include/secure/_stdio.h:49:28: note: expanded from macro 'sprintf'
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
                           ^
erl_memory.c:1109:23: warning: passing 'usgnd_int_8 *' (aka 'unsigned char *') to parameter of type 'char *' converts between pointers to integer types with different sign [-Wpointer-sign]
        area.size += sprintf(area.ptr + area.size,
                             ^~~~~~~~~~~~~~~~~~~~
/usr/include/secure/_stdio.h:49:28: note: expanded from macro 'sprintf'
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
                           ^
erl_memory.c:1120:26: warning: passing 'usgnd_int_8 *' (aka 'unsigned char *') to parameter of type 'char *' converts between pointers to integer types with different sign [-Wpointer-sign]
    area.size += sprintf(area.ptr + area.size,
                         ^~~~~~~~~~~~~~~~~~~~
/usr/include/secure/_stdio.h:49:28: note: expanded from macro 'sprintf'
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
                           ^
erl_memory.c:1190:18: warning: passing 'usgnd_int_8 *' (aka 'unsigned char *') to parameter of type 'char *' converts between pointers to integer types with different sign [-Wpointer-sign]
    p += sprintf(p, "> %-*s", EM_TIME_FIELD_WIDTH - 2, "Maximum:");
                 ^
/usr/include/secure/_stdio.h:49:28: note: expanded from macro 'sprintf'
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
                           ^
erl_memory.c:1223:18: warning: passing 'usgnd_int_8 *' (aka 'unsigned char *') to parameter of type 'char *' converts between pointers to integer types with different sign [-Wpointer-sign]
    p += sprintf(p, "\n");
                 ^
/usr/include/secure/_stdio.h:49:28: note: expanded from macro 'sprintf'
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
                           ^
erl_memory.c:1227:15: warning: passing 'usgnd_int_8 *' (aka 'unsigned char *') to parameter of type 'char *' converts between pointers to integer types with different sign [-Wpointer-sign]
        p += sprintf(p, "%s", stop_str);
                     ^
/usr/include/secure/_stdio.h:49:28: note: expanded from macro 'sprintf'
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
                           ^
erl_memory.c:1230:15: warning: passing 'usgnd_int_8 *' (aka 'unsigned char *') to parameter of type 'char *' converts between pointers to integer types with different sign [-Wpointer-sign]
        p += sprintf(p, exit_str, state->info.exit_status);
                     ^
/usr/include/secure/_stdio.h:49:28: note: expanded from macro 'sprintf'
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
                           ^
erl_memory.c:1236:18: warning: passing 'usgnd_int_8 *' (aka 'unsigned char *') to parameter of type 'char *' converts between pointers to integer types with different sign [-Wpointer-sign]
    p += sprintf(p, format, tsz, bw);
                 ^
/usr/include/secure/_stdio.h:49:28: note: expanded from macro 'sprintf'
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
                           ^
erl_memory.c:1286:18: warning: passing 'usgnd_int_8 *' (aka 'unsigned char *') to parameter of type 'char *' converts between pointers to integer types with different sign [-Wpointer-sign]
    p += sprintf(p, "%*" USGND_INT_32_FSTR " ", EM_TIME_FIELD_WIDTH - 1, secs);
                 ^
/usr/include/secure/_stdio.h:49:28: note: expanded from macro 'sprintf'
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
                           ^
erl_memory.c:1311:18: warning: passing 'usgnd_int_8 *' (aka 'unsigned char *') to parameter of type 'char *' converts between pointers to integer types with different sign [-Wpointer-sign]
    p += sprintf(p, "\n");
                 ^
/usr/include/secure/_stdio.h:49:28: note: expanded from macro 'sprintf'
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
                           ^
erl_memory.c:2231:32: warning: passing 'char *' to parameter of type 'usgnd_int_8 *' (aka 'unsigned char *') converts between pointers to integer types with different sign [-Wpointer-sign]
    size = write_header(state, state->output.header, 1);
                               ^~~~~~~~~~~~~~~~~~~~
erl_memory.c:736:44: note: passing argument to parameter 'ptr' here
write_header(em_state *state, usgnd_int_8 *ptr, int trunc)
                                           ^
fae3363
@nox nox Silence a mismatching type pointer warning in ei_resolve
Why we are assigning a pointer to pointer to char to a pointer to char
baffles me. The warning is:

connect/ei_resolve.c:229:11: warning: incompatible pointer types assigning to 'char *' from 'char **'; dereference
      with * [-Wincompatible-pointer-types]
    *pptr = src_aliases;
          ^ ~~~~~~~~~~~
            *
3029591
@nox nox Conditionally compile my_gethostbyname_r and my_gethostbyaddr_r
These are not needed when not on VXWORKS or without _REENTRANT.

connect/ei_resolve.c:302:24: warning: unused function 'my_gethostbyname_r' [-Wunused-function]
static struct hostent *my_gethostbyname_r(const char *name,
                       ^
connect/ei_resolve.c:360:24: warning: unused function 'my_gethostbyaddr_r' [-Wunused-function]
static struct hostent *my_gethostbyaddr_r(const char *addr,
                       ^
15f3fc6
@nox nox Silence two warnings about tests being always true
encode/encode_ulonglong.c:55:25: warning: comparison of unsigned expression >= 0 is always true [-Wtautological-compare]
    if ((p < 256) && (p >= 0)) {
                      ~ ^  ~

legacy/erl_marshal.c:293:24: warning: comparison of unsigned expression >= 0 is always true [-Wtautological-compare]
        if ((ul < 256) && (ul >= 0)) {
                           ~~ ^  ~
fc01fbd
@nox nox Fix some silent rules in diameter, eunit and wx 0817771
@nox nox Compile in_heapfrag() only in debug mode e308ec0
@nox
nox commented Oct 18, 2013

@fenollp Amended.

@nox
nox commented Oct 18, 2013

I wonder if the data read by emtp_parse() is only ASCII too, if it is the memory trace parser should also be patched to just use char *.

@fenollp
fenollp commented Oct 18, 2013

Nice, cheers. I wonder where this should be discussed as they are not really a lot of people that hang around pull request comments. The mailing list maybe?

@nox
nox commented Oct 18, 2013

Well, you are preaching to the choir here. I loathe these pull requests' comments. But unfortunately it seems the Erlang/OTP team is hell-bent on using them now.

@OTP-Maintainer

Patch has passed first testings and has been assigned to be reviewed

@psyeugenic
Erlang/OTP member

We're using Pull Request because the community, i.e. you, asked for it. =) I rather like it because thoughts and code are located at the same place. We should also get rid of the ML so we don't split the information flow imho.

This patch has a lot of changes on very varying places in the code, i.e. different maintainers. Most of the changes seems to be in debug enabled code only, i.e. low priority.

I will get back to you with feedback once I've goon through it properly.

@nox
nox commented Oct 21, 2013

Please note that the third commit is not just warning silencing, it's an actual bug. Most of the changes is not in debug-enabled code, no.

Otherwise while pull requests are good in theory, @OTP-Maintainer gives a good example of where it sucks big time, you have to take care of formatting your emails etc, and Markdown is underspecified. I would rather have plain text emails which I can browse offline. I don't even have my own comments received by email when using pull requests, that's splitting information.

We gave up on standard email protocols to use a proprietary forum, this sucks.

And I have never asked for pull requests.

@nox
nox commented Oct 21, 2013

Only the first three commits and the last one change debug-only code.

@nox
nox commented Oct 21, 2013

Also, while someone can't delete an email that I have already received, anyone can delete a comment he made on GitHub. I fail to see that as an improvement.

And people can also amend their comments whenever they want, this is insane.

@psyeugenic
Erlang/OTP member

You should voice your opinion on Erlang Questions then. We certainly want feedback. GitHub has shortcomings and ML's certainly have also.

@nox
nox commented Oct 21, 2013

Done. Now I am bracing for impact.

@proxyles

Graduated

@proxyles proxyles closed this Jan 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.