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

{.noinit.} ignored in for loop -> bad codegen for non-movable types #14118

Closed
mratsim opened this issue Apr 25, 2020 · 0 comments
Closed

{.noinit.} ignored in for loop -> bad codegen for non-movable types #14118

mratsim opened this issue Apr 25, 2020 · 0 comments

Comments

@mratsim
Copy link
Collaborator

mratsim commented Apr 25, 2020

In some situation, in particular in a for loop, the compiler may ignore the {.noInit.} pragma.

This is problematic for non-copyable non-movable type, for example a shared atomic counter.
This can be caught at compile-time by making =sink an error:

import std/atomics

type AtomicCounter* = object
  count: Atomic[int]

proc `=`*(dst: var AtomicCounter, src: AtomicCounter) {.error: "An atomic counter cannot be copied.".}

proc `=sink`*(dst: var AtomicCounter, src: AtomicCounter) {.error: "An atomic counter cannot be moved.".}

proc `=destroy`*(sb: var AtomicCounter) {.inline.}=
  discard

proc initialize*(ac: var AtomicCounter) {.inline.} =
  ac.count.store(0, moRelaxed)

proc inc*(p: ptr AtomicCounter) =
  discard p.count.fetchAdd(1, moRelaxed)

var currentCounter: ptr AtomicCounter

template scopedCounter*(body: untyped): untyped =
  bind currentCounter
  block:
    let suspended = currentCounter
    var a {.noInit.}: AtomicCounter
    a.initialize()
    currentCounter = addr(a)

    body

    currentCounter = suspended

proc foo() =
  scopedCounter:
    echo "Counting ..."

proc bar() =
  for i in 0 .. 2:
    scopedCounter:
      echo "Counting ..."

foo()
bar() # Does not compile, Nim tries to insert `=sink` inside a for loop
Error: '=sink' is not available for type <AtomicCounter>; routine: bar

In the C codegen we can see the eqsink

import std/atomics

type AtomicCounter* = object
  count: Atomic[int]

proc `=`*(dst: var AtomicCounter, src: AtomicCounter) {.error: "An atomic counter cannot be copied.".}

when false:
  proc `=sink`*(dst: var AtomicCounter, src: AtomicCounter) {.error: "An atomic counter cannot be moved.".}
else:
  proc `=sink`*(dst: var AtomicCounter, src: AtomicCounter) {.inline.} =
    system.`=sink`(dst.count, src.count)

proc `=destroy`*(sb: var AtomicCounter) {.inline.}=
  discard

proc initialize*(ac: var AtomicCounter) {.inline.} =
  ac.count.store(0, moRelaxed)

proc inc*(p: ptr AtomicCounter) =
  discard p.count.fetchAdd(1, moRelaxed)

var currentCounter: ptr AtomicCounter

template scopedCounter*(body: untyped): untyped =
  bind currentCounter
  block:
    let suspended = currentCounter
    var a {.noInit.}: AtomicCounter
    a.initialize()
    currentCounter = addr(a)

    body

    currentCounter = suspended

proc foo() =
  scopedCounter:
    echo "Counting ..."

proc bar() =
  for i in 0 .. 2:
    scopedCounter:
      echo "Counting ..."

foo()
bar() # eqsink in C codegen

foo function

N_LIB_PRIVATE N_NIMCALL(void, foo__JzbnjO5RDbS420V3wzhF6Q)(void) {
	tyObject_AtomicCounter__Yz7FoDTBlGPGeC6wQ9aoO7Q aX60gensym5240276_;
	TSafePoint TM__9aQ9cEDplzy5RH1NH07Dxehw_2;
	pushSafePoint(&TM__9aQ9cEDplzy5RH1NH07Dxehw_2);
	TM__9aQ9cEDplzy5RH1NH07Dxehw_2.status = setjmp(TM__9aQ9cEDplzy5RH1NH07Dxehw_2.context);
	if (TM__9aQ9cEDplzy5RH1NH07Dxehw_2.status == 0) {
		{
			tyObject_AtomicCounter__Yz7FoDTBlGPGeC6wQ9aoO7Q* suspendedX60gensym5240275_;
			suspendedX60gensym5240275_ = currentCounter__uTp0tKFcbHqRY13s6w9cfig;
			initialize__5UeDB2gx8kATVzHYGeWFRAnoinit_custom_destructors((&aX60gensym5240276_));
			currentCounter__uTp0tKFcbHqRY13s6w9cfig = (&aX60gensym5240276_);
			echoBinSafe(TM__9aQ9cEDplzy5RH1NH07Dxehw_3, 1);
			currentCounter__uTp0tKFcbHqRY13s6w9cfig = suspendedX60gensym5240275_;
		}
		popSafePoint();
	}
	else {
		popSafePoint();
	}
	{
		eqdestroy___DjicWcWwV2o4NWG9aKCOq3gnoinit_custom_destructors((&aX60gensym5240276_));
		if (TM__9aQ9cEDplzy5RH1NH07Dxehw_2.status != 0) nimLeaveFinally();
	}
	if (TM__9aQ9cEDplzy5RH1NH07Dxehw_2.status != 0) reraiseException();
}

bar function

N_LIB_PRIVATE N_NIMCALL(void, bar__JzbnjO5RDbS420V3wzhF6Q_2)(void) {
	tyObject_AtomicCounter__Yz7FoDTBlGPGeC6wQ9aoO7Q aX60gensym5245035_;
	TSafePoint TM__9aQ9cEDplzy5RH1NH07Dxehw_5;
	pushSafePoint(&TM__9aQ9cEDplzy5RH1NH07Dxehw_5);
	TM__9aQ9cEDplzy5RH1NH07Dxehw_5.status = setjmp(TM__9aQ9cEDplzy5RH1NH07Dxehw_5.context);
	if (TM__9aQ9cEDplzy5RH1NH07Dxehw_5.status == 0) {
		{
			NI i;
			NI res;
			i = (NI)0;
			res = ((NI) 0);
			{
				while (1) {
					NI TM__9aQ9cEDplzy5RH1NH07Dxehw_6;
					if (!(res <= ((NI) 2))) goto LA4;
					i = res;
					{
						tyObject_AtomicCounter__Yz7FoDTBlGPGeC6wQ9aoO7Q* suspendedX60gensym5245034_;
						tyObject_AtomicCounter__Yz7FoDTBlGPGeC6wQ9aoO7Q T6_;
						suspendedX60gensym5245034_ = currentCounter__uTp0tKFcbHqRY13s6w9cfig;
						nimZeroMem((void*)(&T6_), sizeof(tyObject_AtomicCounter__Yz7FoDTBlGPGeC6wQ9aoO7Q));
						eqsink___f9azodprU2vXmRJy7KoSVUAnoinit_custom_destructors((&aX60gensym5245035_), T6_);
						initialize__5UeDB2gx8kATVzHYGeWFRAnoinit_custom_destructors((&aX60gensym5245035_));
						currentCounter__uTp0tKFcbHqRY13s6w9cfig = (&aX60gensym5245035_);
						echoBinSafe(TM__9aQ9cEDplzy5RH1NH07Dxehw_3, 1);
						currentCounter__uTp0tKFcbHqRY13s6w9cfig = suspendedX60gensym5245034_;
					}
					if (nimAddInt(res, ((NI) 1), &TM__9aQ9cEDplzy5RH1NH07Dxehw_6)) { raiseOverflow(); };
					res = (NI)(TM__9aQ9cEDplzy5RH1NH07Dxehw_6);
				} LA4: ;
			}
		}
		popSafePoint();
	}
	else {
		popSafePoint();
	}
	{
		eqdestroy___DjicWcWwV2o4NWG9aKCOq3gnoinit_custom_destructors((&aX60gensym5245035_));
		if (TM__9aQ9cEDplzy5RH1NH07Dxehw_5.status != 0) nimLeaveFinally();
	}
	if (TM__9aQ9cEDplzy5RH1NH07Dxehw_5.status != 0) reraiseException();
}

More importantly, this will avoid bad codegen in C++ that only happen for bar but not for foo


........./nimcache/atcounter/@mnoinit_custom_destructors.nim.cpp: In function ‘void eqsink___f9azodprU2vXmRJy7KoSVUAnoinit_custom_destructors(tyObject_AtomicCounter__Yz7FoDTBlGPGeC6wQ9aoO7Q&, tyObject_AtomicCounter__Yz7FoDTBlGPGeC6wQ9aoO7Q)’:
........./nimcache/atcounter/@mnoinit_custom_destructors.nim.cpp:126:18: error: use of deleted function ‘std::atomic<long int>& std::atomic<long int>::operator=(const std::atomic<long int>&)’
  126 |  dst.count = src.count;
      |                  ^~~~~
In file included from ........./nimcache/atcounter/@mnoinit_custom_destructors.nim.cpp:10:
/usr/include/c++/9.3.0/atomic:802:15: note: declared here
  802 |       atomic& operator=(const atomic&) = delete;
      |               ^~~~~~~~
........./nimcache/atcounter/@mnoinit_custom_destructors.nim.cpp: In function ‘void bar__JzbnjO5RDbS420V3wzhF6Q_2()’:
........./nimcache/atcounter/@mnoinit_custom_destructors.nim.cpp:150:88: error: use of deleted function ‘tyObject_AtomicCounter__Yz7FoDTBlGPGeC6wQ9aoO7Q::tyObject_AtomicCounter__Yz7FoDTBlGPGeC6wQ9aoO7Q(const tyObject_AtomicCounter__Yz7FoDTBlGPGeC6wQ9aoO7Q&)’
  150 |       eqsink___f9azodprU2vXmRJy7KoSVUAnoinit_custom_destructors(aX60gensym5215035_, T8_);
      |                                                                                        ^
........./nimcache/atcounter/@mnoinit_custom_destructors.nim.cpp:38:8: note: ‘tyObject_AtomicCounter__Yz7FoDTBlGPGeC6wQ9aoO7Q::tyObject_AtomicCounter__Yz7FoDTBlGPGeC6wQ9aoO7Q(const tyObject_AtomicCounter__Yz7FoDTBlGPGeC6wQ9aoO7Q&)’ is implicitly deleted because the default definition would be ill-formed:
   38 | struct tyObject_AtomicCounter__Yz7FoDTBlGPGeC6wQ9aoO7Q {
      |        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
........./nimcache/atcounter/@mnoinit_custom_destructors.nim.cpp:38:8: error: use of deleted function ‘std::atomic<long int>::atomic(const std::atomic<long int>&)’
In file included from ........./nimcache/atcounter/@mnoinit_custom_destructors.nim.cpp:10:
/usr/include/c++/9.3.0/atomic:801:7: note: declared here
  801 |       atomic(const atomic&) = delete;
      |       ^~~~~~
........./nimcache/atcounter/@mnoinit_custom_destructors.nim.cpp:125:184: note:   initializing argument 2 of ‘void eqsink___f9azodprU2vXmRJy7KoSVUAnoinit_custom_destructors(tyObject_AtomicCounter__Yz7FoDTBlGPGeC6wQ9aoO7Q&, tyObject_AtomicCounter__Yz7FoDTBlGPGeC6wQ9aoO7Q)’
  125 | static N_INLINE(void, eqsink___f9azodprU2vXmRJy7KoSVUAnoinit_custom_destructors)(tyObject_AtomicCounter__Yz7FoDTBlGPGeC6wQ9aoO7Q& dst, tyObject_AtomicCounter__Yz7FoDTBlGPGeC6wQ9aoO7Q src) {
      |                                                                                                                                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
Error: execution of an external compiler program 'g++ -c  -w -w -fpermissive -pthread -O3 -fno-strict-aliasing -fno-ident -std=gnu++14 -funsigned-char  -I/home/beta/.choosenim/toolchains/nim-1.2.0/lib -I......... -o ........./nimcache/atcounter/@mnoinit_custom_destructors.nim.cpp.o ........./nimcache/atcounter/@mnoinit_custom_destructors.nim.cpp' failed with exit code: 1

This may be related to my C++ codegen woes in #13093 which I have no workaround for at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant