Permalink
Browse files

LUA: Add assert()s in several places to make clang analyzer happy

  • Loading branch information...
DrMcCoy committed Nov 5, 2015
1 parent 2a467ec commit bfe06f4c357df2ddaf0ff6ee0b44ef9ff654873c
Showing with 8 additions and 0 deletions.
  1. +2 −0 lua/liolib.c
  2. +2 −0 lua/lobject.h
  3. +2 −0 lua/lparser.c
  4. +2 −0 lua/lstate.c
View
@@ -5,6 +5,7 @@
*/
#include <assert.h>
#include <errno.h>
#include <locale.h>
#include <stdio.h>
@@ -397,6 +398,7 @@ static int g_read (lua_State *L, FILE *f, int first) {
else {
const char *p = lua_tostring(L, n);
luaL_argcheck(L, p && p[0] == '*', n, "invalid option");
assert(p);

This comment has been minimized.

Show comment
Hide comment
@turol

turol Nov 5, 2015

This one and the changes to lparser.c and lstate.c could be avoided by marking luaD_throw and its callers as noreturn. This way the analyzer would automatically see they're impossible. Also it would probably allow removing the unnecessary return-statement from lua_error in lapi.c 808.

@turol

turol Nov 5, 2015

This one and the changes to lparser.c and lstate.c could be avoided by marking luaD_throw and its callers as noreturn. This way the analyzer would automatically see they're impossible. Also it would probably allow removing the unnecessary return-statement from lua_error in lapi.c 808.

This comment has been minimized.

Show comment
Hide comment
@turol

turol Nov 5, 2015

Here's an example how to do this and how the necessary changes can "cascade" through functions:

turol/webquake2@259cf75

@turol

turol Nov 5, 2015

Here's an example how to do this and how the necessary changes can "cascade" through functions:

turol/webquake2@259cf75

switch (p[1]) {
case 'n': /* number */
success = read_number(L, f);
View
@@ -7,6 +7,7 @@
#ifndef lobject_h
#define lobject_h
#include <assert.h>
#include "llimits.h"
#include "lua.h"
@@ -143,6 +144,7 @@ typedef struct lua_TObject {
#define setobj(obj1,obj2) \
{ const TObject *o2=(obj2); TObject *o1=(obj1); \
checkconsistency(o2); \
assert(o2); \

This comment has been minimized.

Show comment
Hide comment
@turol

turol Nov 5, 2015

This should be above checkconsistency call since it will also dereference o2.

@turol

turol Nov 5, 2015

This should be above checkconsistency call since it will also dereference o2.

This comment has been minimized.

Show comment
Hide comment
@DrMcCoy

DrMcCoy Nov 5, 2015

Member

Oh, right, I missed that. But so does clang analyzer, it seems. :)

@DrMcCoy

DrMcCoy Nov 5, 2015

Member

Oh, right, I missed that. But so does clang analyzer, it seems. :)

This comment has been minimized.

Show comment
Hide comment
@turol

turol Nov 5, 2015

You should never blindly add stuff just to silence the analyzer but instead try to understand what the code does and what the analyzer is complaining about. I have blown up code before by trying to eliminate all compiler warnings without understanding the code.

@turol

turol Nov 5, 2015

You should never blindly add stuff just to silence the analyzer but instead try to understand what the code does and what the analyzer is complaining about. I have blown up code before by trying to eliminate all compiler warnings without understanding the code.

This comment has been minimized.

Show comment
Hide comment
@DrMcCoy

DrMcCoy Nov 5, 2015

Member

You're right.

I was basically assuming clang was being oversensitive, because Lua is "obviously" correct. That was premature and impatient of me.

I'll revert this, and see if marking luaD_throw as non-returning makes a difference.

@DrMcCoy

DrMcCoy Nov 5, 2015

Member

You're right.

I was basically assuming clang was being oversensitive, because Lua is "obviously" correct. That was premature and impatient of me.

I'll revert this, and see if marking luaD_throw as non-returning makes a difference.

This comment has been minimized.

Show comment
Hide comment
@turol

turol Nov 5, 2015

You'll also need to mark everyone who unconditionally calls luaD_throw and all their callers. Just make sure they do so unconditionally, otherwise you will get nasty bugs.

@turol

turol Nov 5, 2015

You'll also need to mark everyone who unconditionally calls luaD_throw and all their callers. Just make sure they do so unconditionally, otherwise you will get nasty bugs.

This comment has been minimized.

Show comment
Hide comment
@clone2727

clone2727 Nov 5, 2015

Contributor

No, compilers are smarter than that. They're able to optimize for the noreturn case, but it never breaks the caller.

@clone2727

clone2727 Nov 5, 2015

Contributor

No, compilers are smarter than that. They're able to optimize for the noreturn case, but it never breaks the caller.

This comment has been minimized.

Show comment
Hide comment
@turol

turol Nov 5, 2015

https://gcc.gnu.org/onlinedocs/gcc-5.2.0/gcc/Function-Attributes.html#Function-Attributes

"The noreturn keyword tells the compiler to assume that fatal cannot return. It can then optimize without regard to what would happen if fatal ever did return."

So if you mark a function "noreturn" but it can return you will break code. So if you have code like this:

if (some_error_condition) {
  fatal_func();
}

return success;

it must not be marked as noreturn. Whereas this is correct:

void some_error() __attribute__((noreturn)) {
  printf("fail\n");
  exit(1);
}
@turol

turol Nov 5, 2015

https://gcc.gnu.org/onlinedocs/gcc-5.2.0/gcc/Function-Attributes.html#Function-Attributes

"The noreturn keyword tells the compiler to assume that fatal cannot return. It can then optimize without regard to what would happen if fatal ever did return."

So if you mark a function "noreturn" but it can return you will break code. So if you have code like this:

if (some_error_condition) {
  fatal_func();
}

return success;

it must not be marked as noreturn. Whereas this is correct:

void some_error() __attribute__((noreturn)) {
  printf("fail\n");
  exit(1);
}

This comment has been minimized.

Show comment
Hide comment
@clone2727

clone2727 Nov 5, 2015

Contributor

Yes. I'm saying not marking something as noreturn will not cause a bug.

@clone2727

clone2727 Nov 5, 2015

Contributor

Yes. I'm saying not marking something as noreturn will not cause a bug.

This comment has been minimized.

Show comment
Hide comment
@turol

turol Nov 5, 2015

Ah, I misunderstood you. But like we saw here, not marking stuff noreturn will cause spurious warnings.

@turol

turol Nov 5, 2015

Ah, I misunderstood you. But like we saw here, not marking stuff noreturn will cause spurious warnings.

o1->tt=o2->tt; o1->value = o2->value; }
View
@@ -5,6 +5,7 @@
*/
#include <assert.h>
#include <string.h>
#define lparser_c
@@ -1258,6 +1259,7 @@ static void breakstat (LexState *ls) {
}
if (!bl)
luaX_syntaxerror(ls, "no loop to break");
assert(bl);
if (upval)
luaK_codeABC(fs, OP_CLOSE, bl->nactvar, 0, 0);
luaK_concat(fs, &bl->breaklist, luaK_jump(fs));
View
@@ -5,6 +5,7 @@
*/
#include <assert.h>
#include <stdlib.h>
#define lstate_c
@@ -91,6 +92,7 @@ static void f_luaopen (lua_State *L, void *ud) {
global_State *g = luaM_new(NULL, global_State);
UNUSED(ud);
if (g == NULL) luaD_throw(L, LUA_ERRMEM);
assert(g);
L->l_G = g;
g->mainthread = L;
g->GCthreshold = 0; /* mark it as unfinished state */

0 comments on commit bfe06f4

Please sign in to comment.