v

v copied to clipboard

See test case, there are some tests that are not passing because the check is not done properly for some scopes.

Also I think it would be interesting to avoid having those checks for unsafe {} blocks.

See the test as an example

Current fail:

/tmp/v/v2.15775295321930989009.tmp.c: In function ‘array map_keys(map*)’:
26
/tmp/v/v2.15775295321930989009.tmp.c:14836:62: error: second operand to the conditional operator is of type ‘void’, but the third operand is neither a throw-expression nor of type ‘void’
27
14836 |     /* null check */ (m->clone_fn)? m->clone_fn(item, pkey): 0;
28
      |                                                              ^

I'm happy if anyone with more knowledge on the compiler internals and spare time can take my PR and finish it+merge. otherwise i will need to clarify the following things:

• How to check if the call is inside an unsafe{} block?
• Do we want to get a warning at runtime (or assert) instead of silently not calling the function if the pointer is null?
• Should this catch or {} blocks for this too?
• May -prod builds not check for this?

We were considering optional function fields or [required] fields to solve this issue:

https://github.com/vlang/v/discussions/7610#discussioncomment-257982

Hiding null functions like this can lead to hard to debug bugs.

What would this do for functions that return values?

i have updated the PR description to respond your questions. i have some more questions too :D but i think it's worth discussing/clarifying to address this

It may be more general to generate dummy functions, that have the same signature, but that just return the default value for the return type, and initialize the fn callback fields with those, instead of 0. This will not need checks and skips at each call site, and will not segfault.

How to check if the call is inside an unsafe{} block?

Currently the cgen stage can not check that efficiently, unlike the checker, but it is easy to add. You need to add a new field in struct Gen, around vlib/v/gen/c/cgen.v:77 , say is_unsafe bool, then modify vlib/v/gen/c/cgen.v:3231 to set g.is_unsafe to true, and g.is_unsafe = false afterwards, same with vlib/v/gen/c/cgen.v:1085 .

just updated the pr, didnt had much spare time. the unsafe check is not clear to me because the rebasing changed the line numbers. But still, forcing true/false on some scopes doesnt seems like honoring the user defined unsafe blocks :?

./v -cc g++-9 -o v2 cmd/v && ./v2 -cc g++-9 -o v3 cmd/v
==================
/tmp/v/v2.11056817583672041563.tmp.c: In function ‘void map_set(map*, voidptr, voidptr)’:
/tmp/v/v2.11056817583672041563.tmp.c:15043:59: error: second operand to the conditional operator is of type ‘void’, but the third operand is neither a throw-expression nor of type ‘void’
15043 |   /* null check */ (m->clone_fn)? m->clone_fn(pkey, key): 0;

I'm all for @spytheman's solution, generating dummy function and assigning them by default, rather than having this check everytime the function is called

The problem i see in this approach is that silently calling a null pointer function and getting a value in exchange doesn't seems correct to me. I see some more issues to cover this properly.

• All function pointers should be treated as [unsafe]
• Calling an unsafe function should be only happening from inside unsafe { blocks
• The [unsafe] tag is ignored in a variety of situations and there's no easy way to

Additionaly i think that after making all this we need to clarify:

• As long as the function call is unsafe, the user should be in charge to check for the null
• Return optional so the user can handle those errors programatically (maybe this invalidates the unsafe block requirement)
• Blindly return 0/null/empty-string/.. which seems wrong to me, despite being the most practical solution.

The issue aiming t solve this pr is in the roadmap. Would be good to know what would be the path to take here ^^

ping?

@trufae Hi, is this still actual? Should somebody from the community rebase this PR and try to merge it (in case you don't have time/are not interested in it anymore)

@trufae Can we get some example code, to see how the current version V does against what you were trying to solve? Since you started this, V has gotten the Option fields discussed, etc.

Running the test file with the latest version of V gives the following output:

---- Testing... --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 FAIL   120.412 ms /home/jalon/struct_fields_storing_functions_test.v
[136, 1, 2, 2, 153]
RUNTIME ERROR: invalid memory access
hello world

------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Failed command 1:    '/home/jalon/git/v/v'   -o '/tmp/v_1000/tsession_7f82478aa740_184466969129049/struct_fields_storing_functions_test' '/home/jalon/struct_fields_storing_functions_test.v'
Summary for all V _test.v files: 1 failed, 1 total. Runtime: 120 ms, on 1 job.

So... no more segfaults, at least. I haven't delved far enough in to see why it gets the invalid memory access, but that could simply be the null pointer.

Unless someone comes up with a good reason otherwise, I plan to close this PR soon, as "no longer needed".

@spytheman @medvednikov @trufae

No comments, I think this can be closed