Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

builtin.c: typecheck builtin cfunctions in function_list #3209

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

emanuele6
Copy link
Member

@emanuele6 emanuele6 commented Nov 25, 2024

In C23 (default C standard used by GCC 15), jv (*fptr)(); has become equivalent to jv (*fptr)(void); so we can no longer assign builtin implemenations directly to the fptr member of cfunctions without generating a compile error.

Since there does not seem to be any straight-forward way to tell autoconf to force the compiler to use C99 short of explicitly adding -std=c99 to CFLAGS, it is probably a cleaner solution to just make the code C23 compatible.

A possible solution could have been to just redeclare cfunction.fptr as void*, but then the functions' return type would not have been type checked (e.g. if you tried to add a {printf, "printf", 2}, where printf is a function that does not return jv, the compiler wouldn't have complained.)

We were already not typechecking the arguments of the functions, so e.g.

{binop_plus, "_plus", 3},  /* instead of {f_plus, "_plus, 3},       */
{f_setpath, "setpath", 4}, /* instead of {f_setpath, "setpath", 3}, */

compile without errors despite not having the correct prototype.

So I thought of instead improving the situation by redefining cfunction.fptr as a union of function pointers with the prototypes that the jq bytecode interpreter can call, and use a macro to add the builtin functions to function_list using to the arity argument to assign the implementation function to the appropriate union member.

Now the code won't compile if the wrong arity, or an arity not supported by the bytecode interpreter (>5 = 1input+4arguments), or a prototype not jallable by the bytecode interpreter (e.g. binop_plus that doesn't expect a jq_state* argument).

$ make -j
make  all-recursive
make[1]: Entering directory '/home/emanuele6/git/jq'
make[2]: Entering directory '/home/emanuele6/git/jq'
  CC       src/builtin.lo
src/builtin.c:1831:9: error: initialization of 'jv (*)(jq_state *, jv,  jv,  jv,  jv)' from incompatible pointer type 'jv (*)(jq_state *, jv,  jv,  jv)' [-Wincompatible-pointer-types]
 1831 |   CFUNC(f_setpath, "setpath", 4),
      |         ^~~~~~~~~
src/builtin.c:1794:28: note: in definition of macro 'CFUNC'
 1794 |   {.fptr = { .a ## arity = func }, name, arity}
      |                            ^~~~
src/builtin.c:1831:9: note: (near initialization for 'function_list[88].fptr.a4')
 1831 |   CFUNC(f_setpath, "setpath", 4),
      |         ^~~~~~~~~
src/builtin.c:1794:28: note: in definition of macro 'CFUNC'
 1794 |   {.fptr = { .a ## arity = func }, name, arity}
      |                            ^~~~
make[2]: *** [Makefile:1056: src/builtin.lo] Error 1
make[2]: Leaving directory '/home/emanuele6/git/jq'
make[1]: *** [Makefile:1193: all-recursive] Error 1
make[1]: Leaving directory '/home/emanuele6/git/jq'
$ make
mkdir -p src
  GEN      src/version.h
make  all-recursive
make[1]: Entering directory '/home/emanuele6/git/jq'
make[2]: Entering directory '/home/emanuele6/git/jq'
  CC       src/main.o
  CC       src/builtin.lo
src/builtin.c:1794:15: error: 'union <anonymous>' has no member named 'a6'; did you mean 'a1'?
 1794 |   {.fptr = { .a ## arity = func }, name, arity}
      |               ^
src/builtin.c:1821:3: note: in expansion of macro 'CFUNC'
 1821 |   CFUNC(f_test6, "test6", 6),
      |   ^~~~~
src/builtin.c:1821:9: error: initialization of 'jv (*)(jq_state *, jv)' from incompatible pointer type 'jv (*)(jq_state *, jv,  jv,  jv,  jv,  jv,  jv)' [-Wincompatible-pointer-types]
 1821 |   CFUNC(f_test6, "test6", 6),
      |         ^~~~~~~
src/builtin.c:1794:28: note: in definition of macro 'CFUNC'
 1794 |   {.fptr = { .a ## arity = func }, name, arity}
      |                            ^~~~
src/builtin.c:1821:9: note: (near initialization for 'function_list[73].fptr.a1')
 1821 |   CFUNC(f_test6, "test6", 6),
      |         ^~~~~~~
src/builtin.c:1794:28: note: in definition of macro 'CFUNC'
 1794 |   {.fptr = { .a ## arity = func }, name, arity}
      |                            ^~~~
make[2]: *** [Makefile:1056: src/builtin.lo] Error 1
make[2]: Leaving directory '/home/emanuele6/git/jq'
make[1]: *** [Makefile:1193: all-recursive] Error 1
make[1]: Leaving directory '/home/emanuele6/git/jq'
make: *** [Makefile:831: all] Error 2
$ make
make  all-recursive
make[1]: Entering directory '/home/emanuele6/git/jq'
make[2]: Entering directory '/home/emanuele6/git/jq'
  CC       src/builtin.lo
src/builtin.c:1813:9: error: initialization of 'jv (*)(jq_state *, jv,  jv,  jv)' from incompatible pointer type 'jv (*)(jv,  jv)' [-Wincompatible-pointer-types]
 1813 |   CFUNC(binop_plus, "_plus", 3),
      |         ^~~~~~~~~~
src/builtin.c:1794:28: note: in definition of macro 'CFUNC'
 1794 |   {.fptr = { .a ## arity = func }, name, arity}
      |                            ^~~~
src/builtin.c:1813:9: note: (near initialization for 'function_list[62].fptr.a3')
 1813 |   CFUNC(binop_plus, "_plus", 3),
      |         ^~~~~~~~~~
src/builtin.c:1794:28: note: in definition of macro 'CFUNC'
 1794 |   {.fptr = { .a ## arity = func }, name, arity}
      |                            ^~~~
make[2]: *** [Makefile:1056: src/builtin.lo] Error 1
make[2]: Leaving directory '/home/emanuele6/git/jq'
make[1]: *** [Makefile:1193: all-recursive] Error 1
make[1]: Leaving directory '/home/emanuele6/git/jq'
make: *** [Makefile:831: all] Error 2

Also, the code now compiles with gcc -std=c23.

Fixes #3206

@emanuele6 emanuele6 added portability lint Code cleanup; style fixes; small refactors; dead code removal; etc. labels Nov 25, 2024
src/execute.c Outdated Show resolved Hide resolved
@wader
Copy link
Member

wader commented Nov 25, 2024

Would it make sense to have CI pipeline that builds with highest supported C standard version?

In C23 (default C standard used by GCC 15),  jv (*fptr)();  has become
equivalent to  jv (*fptr)(void);  so we can no longer assign builtin
implemenations directly to the fptr member of cfunctions without
generating a compile error.

Since there does not seem to be any straight-forward way to tell
autoconf to force the compiler to use C99 short of explicitly adding
-std=c99 to CFLAGS, it is probably a cleaner solution to just make the
code C23 compatible.

A possible solution could have been to just redeclare  cfunction.fptr
as void*, but then the functions' return type would not have been type
checked (e.g. if you tried to add a {printf, "printf", 2}, where printf
is a function that does not return jv, the compiler wouldn't have
complained.)
We were already not typechecking the arguments of the functions, so e.g.
  {binop_plus, "_plus", 3},  /* instead of {f_plus, "_plus, 3},       */
  {f_setpath, "setpath", 4}, /* instead of {f_setpath, "setpath", 3}, */
compile without errors despite not having the correct prototype.

So I thought of instead improving the situation by redefining
cfunction.fptr as a union of function pointers with the prototypes that
the jq bytecode interpreter can call, and use a macro to add the builtin
functions to function_list using to the arity argument to assign the
implementation function to the appropriate union member.

Now the code won't compile if the wrong arity, or an arity not supported
by the bytecode interpreter (>5 = 1input+4arguments), or a prototype not
jallable by the bytecode interpreter (e.g. binop_plus that doesn't
expect a  jq_state*  argument).

Also, the code now compiles with gcc -std=c23.

Fixes jqlang#3206
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lint Code cleanup; style fixes; small refactors; dead code removal; etc. portability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build failure with GCC 15 (C23)
2 participants