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

{.push checks:off} and related only works at the top level #11852

Closed
ReneSac opened this issue Jul 29, 2019 · 5 comments · Fixed by #21379
Closed

{.push checks:off} and related only works at the top level #11852

ReneSac opened this issue Jul 29, 2019 · 5 comments · Fixed by #21379

Comments

@ReneSac
Copy link
Contributor

ReneSac commented Jul 29, 2019

EDIT: Updated the title to better reflect the issue. Moving the push and pop pragmas outside the function makes the examples bellow work. So you can only apply the with that granularity. So this is a feature request or alternatively to better document and give an error instead of failing silently. Bellow the original post:

Basically the manual example doesn't work. Filled it to make a (non-)working example:

proc foo(x: string, y: int) =
  {.push checks: off}
  var a: ptr char = unsafeAddr(x[y])
  {.pop.}
  if x.len > y:
    echo repr(a)
  else:
    echo x.len + 48

foo("", 0)
foo("abc", 40)

It compiles and runs fine with -d:danger, but fails with -d:debug:

Error: unhandled exception: index out of bounds, the container is empty [IndexError]

or if a non-empty string:

Error: unhandled exception: index 40 not in 0 .. 2 [IndexError]

Same with {.push boundChecks:off} and others, like for example overflow:

proc foo(y:int) =
  {.push overflowChecks: off}
  let b = y + int.high 
  {.pop.}
  echo b

foo(2)

Works compiled with -d:danger but fails compiled with -d:debug with the usual OverflowError.

Maybe it's related with issue #11826.

I'm sure it used to work, but isn't working at least since nim 0.19 IIRC. I'm currently running the latest git version, Nim Compiler Version 0.20.99 [Linux: amd64]

By the way, does the first code above trips in any undefined behavior due to the way Nim translates to C? In Nim I have to write as if I'm accessing unallocated memory, and indeed I get an "Index Error", but it should be simple pointer arithmetic in C with no memory access.

@mratsim
Copy link
Collaborator

mratsim commented Jul 30, 2019

If checks are disabled (with -d:danger) it will be compiled to &x[y]. Nim inserts bound checking when you want to escape with the address of invalid x[y] for all basic Nim types: array/seq/strings. If you want to avoid this behaviour:

  • Use ptr UncheckedArray[T]
  • or only take the address of the first element:
    let a = cast[ptr char](cast[ByteAddress](x[0].unsafeAddr +% y))

@ReneSac
Copy link
Contributor Author

ReneSac commented Jul 30, 2019

This issue is related to my memviews nimble package, that defines it's own bounds checks for views, and must also create views from Nim basic types. Your suggestions don't apply for my use cases.

I might want to disable the bounds check locally because I made a slice of an bigger allocation and to simplify the logic or to improve the performance I want to be able to access a little past the end of my slice, but still inside the valid memory. For example, to unroll certain loops w/o special tail handling, or reduce the number of checks on each iteration of a loop. And I still want to be able to debug the rest of the code around it with proper bounds check.

When creating a view from a basic nim type, I might receive an empty string, and I believe I should output a view of length zero, not make this an error to be checked by the caller. Taking the address of the first element is still an IndexError in that case. The solution might be to introduce a conditional on length zero inputs, as if the pointer is null any arithmetic would be UB in C IIRC.

@mratsim
Copy link
Collaborator

mratsim commented Jul 31, 2019

In the first case, cast to ptr UncheckedArray. In the second case, you need to check for 0-length. Zero-length seq or strings do not guarantee having a buffer allocated so you can't take its address.

@ReneSac
Copy link
Contributor Author

ReneSac commented Aug 1, 2019

You are right.

And why this bug is only tagged with "error messages"? Unlike those other two issues in #11830, this one is affects runtime behavior and performance, not compile time error messages. Part of the performance issue with the compiler in #11860 might be due to this issue, as this feature was created exactly for tagging hot loops for code otherwise compiled with checks on. Within the compiler (but also effect code compiled that uses it) it's used for strutils, math modules and memory allocation/gc. It's probably just silently failing, leaving bad performance and big binaries behind.

Just one place it's supposedly used to alter behavior, but somehow there is no problem? Are the comments wrong and the pragma unnecessary or the pragma is working there?

EDIT: The pragma works at the top level, and all uses in the compiler seems to be that way, so no performance issues with the compiler I think. It's indeed a case of "error message" on the silent failing when using inside functions and such. And documentation. Although it would be nice if it worked in that finer granularity.

@cwpearson
Copy link

cwpearson commented Aug 9, 2019

I am also having a similar problem: a short example follows. Should I open a new issue for this?

Define two functions to add 1 to an int, one with checks, and one without.
The code generated by both functions is the same: it uses the addInt function, which has overflow checks.

Interestingly, at the top module level, disabling the checks seems to work (the end of the example). It just produces the bare C + operator instead of addInt

# push.nim
# 
# nimc --version
# Nim Compiler Version 0.20.2 [Linux: amd64]
# Compiled at 2019-07-17
# Copyright (c) 2006-2019 by Andreas Rumpf
# 
# git hash: 88a0edba4b1a3d535b54336fd589746add54e937
# active boot switches: -d:release

# compiled with nim c -d:release push.nim

var a: int

# the generated c code for this has runtime checks
proc withChecks*(x: int): int =
    result = x + 1
#[
N_LIB_PRIVATE N_NIMCALL(NI, withChecks_wl09cCE63H1nP4YkQO9bzVEA)(NI x) {
	NI result;
	NI TM_ReqEIoJMnOxilzqn2CD0tQ_2;
	result = (NI)0;
	TM_ReqEIoJMnOxilzqn2CD0tQ_2 = addInt(x, ((NI) 1));
	result = (NI)(TM_ReqEIoJMnOxilzqn2CD0tQ_2);
	return result;
}
]#



# the generated c code for this ALSO has runtime checks
proc withoutChecks*(x: int): int =
    {.push checks: off.}
    result = x + 1
    {.pop.}
#[
N_LIB_PRIVATE N_NIMCALL(NI, withoutChecks_wl09cCE63H1nP4YkQO9bzVEA_2)(NI x) {
	NI result;
	NI TM_ReqEIoJMnOxilzqn2CD0tQ_3;
	result = (NI)0;
	TM_ReqEIoJMnOxilzqn2CD0tQ_3 = addInt(x, ((NI) 1));
	result = (NI)(TM_ReqEIoJMnOxilzqn2CD0tQ_3);
	return result;
}
]#


# this is compiled with checks:
let checked = a + 1
#[
TM_ReqEIoJMnOxilzqn2CD0tQ_4 = addInt(a_fLWbSuYwZs6iXoZ7Sl8Q9ag, ((NI) 1));
checked_IawpvO1ML8qMe3nT4U83cw = (NI)(TM_ReqEIoJMnOxilzqn2CD0tQ_4);
]#


# this is NOT compiled with checks
{.push checks: off.}
let unchecked = a + 1
{.pop.}
#[
unchecked_F0okaOTXyaXjSkZXWzeCZg = (NI)(a_fLWbSuYwZs6iXoZ7Sl8Q9ag + ((NI) 1));
]#


discard withChecks(a)
discard withoutChecks(a)

@ReneSac ReneSac changed the title {.push checks:off} not working {.push checks:off} and related only works at the top level Sep 9, 2019
Araq pushed a commit that referenced this issue Feb 22, 2023
…m now parses the whole module at one time (#21379)

* fixes #19795; remove parse pipeline

* isScript

* fixes nimscriptapi

* don't touch reorder

* check script

* fixes tests

* it seems implicit imports of system cause troubles

* access the first child of `nkStmtList`

* ignore comments

* minor messages

* perhaps increases hloLoopDetector

* the module is a stmtList, which changes the errors

* fixes nimdoc

* fixes tlinter

* fixes nim  secret tests

* fixes arc_misc

* fixes nim secret tests again

* safe; fixes one more test

* GlobalError is the root cause too

* fixes parsing errors

* put emit types to the cfsForwardTypes section

* fixes #11852; `{.push checks:off}` now works in procs

* disable navigator

* fixes nimdoc

* add tests for JS

* fixes nimsuggest
survivorm pushed a commit to survivorm/Nim that referenced this issue Feb 28, 2023
…ove parsing pipeline, Nim now parses the whole module at one time (nim-lang#21379)

* fixes nim-lang#19795; remove parse pipeline

* isScript

* fixes nimscriptapi

* don't touch reorder

* check script

* fixes tests

* it seems implicit imports of system cause troubles

* access the first child of `nkStmtList`

* ignore comments

* minor messages

* perhaps increases hloLoopDetector

* the module is a stmtList, which changes the errors

* fixes nimdoc

* fixes tlinter

* fixes nim  secret tests

* fixes arc_misc

* fixes nim secret tests again

* safe; fixes one more test

* GlobalError is the root cause too

* fixes parsing errors

* put emit types to the cfsForwardTypes section

* fixes nim-lang#11852; `{.push checks:off}` now works in procs

* disable navigator

* fixes nimdoc

* add tests for JS

* fixes nimsuggest
capocasa pushed a commit to capocasa/Nim that referenced this issue Mar 31, 2023
…ove parsing pipeline, Nim now parses the whole module at one time (nim-lang#21379)

* fixes nim-lang#19795; remove parse pipeline

* isScript

* fixes nimscriptapi

* don't touch reorder

* check script

* fixes tests

* it seems implicit imports of system cause troubles

* access the first child of `nkStmtList`

* ignore comments

* minor messages

* perhaps increases hloLoopDetector

* the module is a stmtList, which changes the errors

* fixes nimdoc

* fixes tlinter

* fixes nim  secret tests

* fixes arc_misc

* fixes nim secret tests again

* safe; fixes one more test

* GlobalError is the root cause too

* fixes parsing errors

* put emit types to the cfsForwardTypes section

* fixes nim-lang#11852; `{.push checks:off}` now works in procs

* disable navigator

* fixes nimdoc

* add tests for JS

* fixes nimsuggest
bung87 pushed a commit to bung87/Nim that referenced this issue Jul 29, 2023
…ove parsing pipeline, Nim now parses the whole module at one time (nim-lang#21379)

* fixes nim-lang#19795; remove parse pipeline

* isScript

* fixes nimscriptapi

* don't touch reorder

* check script

* fixes tests

* it seems implicit imports of system cause troubles

* access the first child of `nkStmtList`

* ignore comments

* minor messages

* perhaps increases hloLoopDetector

* the module is a stmtList, which changes the errors

* fixes nimdoc

* fixes tlinter

* fixes nim  secret tests

* fixes arc_misc

* fixes nim secret tests again

* safe; fixes one more test

* GlobalError is the root cause too

* fixes parsing errors

* put emit types to the cfsForwardTypes section

* fixes nim-lang#11852; `{.push checks:off}` now works in procs

* disable navigator

* fixes nimdoc

* add tests for JS

* fixes nimsuggest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants