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

Invalid type in `importc: "exit"' #20694

Closed
arnetheduck opened this issue Oct 29, 2022 · 9 comments · Fixed by #20775
Closed

Invalid type in `importc: "exit"' #20694

arnetheduck opened this issue Oct 29, 2022 · 9 comments · Fixed by #20775
Assignees
Labels

Comments

@arnetheduck
Copy link
Contributor

What happened?

exit takes a cint (32-bit): https://man7.org/linux/man-pages/man3/exit.3.html

quit in Nim takes an int (64-bit):

proc quit*(errorcode: int = QuitSuccess) {.

in reality, only values 0-255 are well supported, but even if the full cint range was ok, a call to quit(0x100000000) will result in a QuitSuccess return code from the application after silent truncation.

Nim Version

534c97e

Current Standard Output Logs

No response

Expected Standard Output Logs

No response

Possible Solution

No response

Additional Information

No response

@ringabout
Copy link
Member

ringabout commented Oct 30, 2022

As the Linux manual says, the use of EXIT_SUCCESS and EXIT_FAILURE is slightly more portable (to non-UNIX environments) than the use of 0 and some nonzero value like 1 or -1.

The only portable values to pass to exit are 0, EXIT_SUCCESS, and EXIT_FAILURE. The latter two are macros defined in <stdlib.h>, the same header that declares the exit function.

The C standard specifies two constants, EXIT_SUCCESS and
EXIT_FAILURE, that may be passed to exit() to indicate successful
or unsuccessful termination, respectively.

in reality, only values 0-255 are well supported, but even if the full cint range was ok, a call to quit(0x100000000) will result in a QuitSuccess return code from the application after silent truncation.

It is not the case on Windows, it supports ranges from 0x0 to 0x3e7f. I think there is no other portable way to implement quit except using QuitSuccess and QuitFailure.

For example, quit(12000) results in a QuitSuccess on Linux, but a QuitFailure on Windows.

How about

@c-blake
Copy link
Contributor

c-blake commented Oct 30, 2022

deprecate the .. only QuitSuccess and QuitFailure

This seems needlessly disruptive. If only 1-bit can really be signaled this way portably, a new enum overload for users wanting portable exit semantics can coexist with the old int way.

While platforms vary, you need not necessarily cater to the lowest common 1-bit denominator. There can be uses for >2 values (a grep one is even in that linked stackoverflow thread). Much of the stdlib tries to abstract what is "commonly" available. As well as "1-bit only exit code" platforms, there are probably also "no folders/directories" platforms, but there is plenty of support for directories and indeed splitPath is at the very top of os.nim. TLDR - perfect portability can be the enemy of good portability.

In that light, the old int signature can maybe also be hardened against silent-fail-to-succeed-conversion by a check used internally but also exported to client code. Then the stdlib can have safer saturating defaults, but a caller can easily handle overflow logic however (including the old ill-advised truncation, maybe for compatibility reasons) -- with the confidence that they & the stdlib agree on the platform range and/or how the checking/clipping is done inside quit if they want to imitate it. E.g. if code notin exitCodeRange: backupChannel.write "forced to truncate " & $code & " to " & exitCodeClip(code). (Those need to handle negative values, BTW, and sure it may be bad to log code only sometimes, not always - it's just an example.)

@arnetheduck
Copy link
Contributor Author

what is "commonly" available

the "common" denominator here is 8 bits, as far as platforms go: it works for linux/bsd/osx/windows etc - windows is "special" in that it supports more bits and the standard fewer - but 8 bits is both safe and reasonably portable (and matches other api like wait.

hardened against silent-fail-to-succeed-conversion

backupChannel is not a workable option unless it's pluggable - randomly writing to the console for example is not a good idea for a core system API (a large proportion of applications do not have a console) - it's also wrong in that it kicks the can to the user of the application, not the developer that used a poorly designed API not knowing better.

this difference needs to appear in the API somehow - ie misusing quit functionality should result in a compile-time failure - one way to do it is indeed to create slightly different API for different platforms, ie quit(v: uint8) for osx/bsd/linux/unixes and quit(v: uint32) for windows - this would mean that if I use an uint8 in my code, it'll be compatible with both win and unix, but using an uint32 will break at compile-time except on windows.

@Araq
Copy link
Member

Araq commented Nov 2, 2022

While I agree that quit(x: enum) or maybe quit(x: uint8) would be the much better design here a bugfix should not require an API redesign. The fix should be that quit still takes an int and that it's saturated to fit into an int8 internally.

@arnetheduck
Copy link
Contributor Author

arnetheduck commented Nov 2, 2022

saturated

saturation is not a bad option in that at least it maintains the "worked/didn't work" aspect of 0 vs non-0 - don't remember if we can overload on static vs non-static x - if yes, we could even add a compile-time warning when the caller specifies a constant that gets saturated.

@ghost
Copy link

ghost commented Nov 2, 2022

@arnetheduck yeah it's possible, e.g.

proc quitMy(x: int) = 
  echo "quitting with ", x

proc quitMy(x: static[int]) = 
  echo "quitting with compile-time ", x

quitMy 5

let a = 3
quitMy a

@c-blake
Copy link
Contributor

c-blake commented Nov 2, 2022

If you do not want to support the larger MS Windows range then you obv. don't need my internal saturation with exposed exitCodeRange/exitCodeClip suggestion, but rather just need to clearly document the saturation since users know the range of int8 or uint8.

FWIW, saturation to 255 is also recommended by The Advanced Bash Scripting Guide: https://stackoverflow.com/questions/1101957/are-there-any-standard-exit-status-codes-in-linux/1535733#1535733

Another trap in this space is that Unix shells can re-code exit-via-signal as negative int8. "Classic" signal numbers only range from 1..31, though.

While that static[int] overload may be nice and I'm in favor since some help is better than none, sadly computed rather than static overflows seem way more common. So, it may still be warranted to have docs mention checking what you pass for overflow and "doing something" (backupChannel was just an example anyone could understand).

@arnetheduck
Copy link
Contributor Author

By the way, everything in this ticket equally applies to the weird programResult global, which also is declared as an int but in reality gets truncated to at least cint (and beyond, ie 8-bit)

Araq pushed a commit that referenced this issue Nov 5, 2022
* quit value gets saturated to ranges

* add documentation

* minimal changes

* refactor

* small fix

* add documentation

* fixes

* Update lib/system.nim

Co-authored-by: Juan Carlos <juancarlospaco@gmail.com>

Co-authored-by: Juan Carlos <juancarlospaco@gmail.com>
@ringabout
Copy link
Member

ringabout commented Nov 7, 2022

  var programResult* {.compilerproc, exportc: "nim_program_result".}: int
    ## deprecated, prefer `quit` or `exitprocs.getProgramResult`, `exitprocs.setProgramResult`.

It seems that programResult has been deprecated. It recommends to use quit or std/exitprocs. Furthermore, it is not an importc function. We could remove it under the umbrella of nimPreviewSlimSystem. As to getProgramResult, it is expected the result is likely to pass to the quit function of which the parameters have been saturated.

capocasa pushed a commit to capocasa/Nim that referenced this issue Mar 31, 2023
* quit value gets saturated to ranges

* add documentation

* minimal changes

* refactor

* small fix

* add documentation

* fixes

* Update lib/system.nim

Co-authored-by: Juan Carlos <juancarlospaco@gmail.com>

Co-authored-by: Juan Carlos <juancarlospaco@gmail.com>
narimiran pushed a commit that referenced this issue Apr 25, 2023
* quit value gets saturated to ranges

* add documentation

* minimal changes

* refactor

* small fix

* add documentation

* fixes

* Update lib/system.nim

Co-authored-by: Juan Carlos <juancarlospaco@gmail.com>

Co-authored-by: Juan Carlos <juancarlospaco@gmail.com>
(cherry picked from commit d5cc208)
narimiran pushed a commit that referenced this issue Apr 25, 2023
* quit value gets saturated to ranges

* add documentation

* minimal changes

* refactor

* small fix

* add documentation

* fixes

* Update lib/system.nim

Co-authored-by: Juan Carlos <juancarlospaco@gmail.com>

Co-authored-by: Juan Carlos <juancarlospaco@gmail.com>
(cherry picked from commit d5cc208)
narimiran pushed a commit that referenced this issue May 12, 2023
* quit value gets saturated to ranges

* add documentation

* minimal changes

* refactor

* small fix

* add documentation

* fixes

* Update lib/system.nim

Co-authored-by: Juan Carlos <juancarlospaco@gmail.com>

Co-authored-by: Juan Carlos <juancarlospaco@gmail.com>
(cherry picked from commit d5cc208)
narimiran pushed a commit that referenced this issue May 12, 2023
bung87 pushed a commit to bung87/Nim that referenced this issue Jul 29, 2023
* quit value gets saturated to ranges

* add documentation

* minimal changes

* refactor

* small fix

* add documentation

* fixes

* Update lib/system.nim

Co-authored-by: Juan Carlos <juancarlospaco@gmail.com>

Co-authored-by: Juan Carlos <juancarlospaco@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants