`GC.malloc` resets `WinError.value` on MinGW-w64

[HertzDevil]

The following seemingly innocuous code:

WinError.value = WinError::ERROR_INVALID_PARAMETER
GC.malloc(1)
p WinError.value

prints WinError::ERROR_INVALID_PARAMETER for MSVC, but WinError::ERROR_SUCCESS for MinGW-w64. The reason is rather subtle:

• Boehm GC on Windows uses different ways to access thread-local storage depending on whether a GNU compiler is used; it defines USE_WIN32_SPECIFIC or USE_WIN32_COMPILER_TLS for GNU and MSVC respectively.
• For MSVC it uses __declspec(thread), and for GNU it uses the internal GC_getspecific, which is a macro for the Win32 TlsGetValue.
• TlsGetValue is one of the few Win32 functions that always set a thread's last error, even if it succeeded.
• This line is where GC_malloc or GC_malloc_atomic ultimately accesses TLS.

Few allocations look as straightforward as the code above. Here is one:

crystal/src/crystal/system/win32/socket.cr

Lines 169 to 173 in 181196f

	private def system_bind(addr, addrstr, &)
	unless LibC.bind(fd, addr, addr.size) == 0
	yield ::Socket::BindError.from_wsa_error("Could not bind to '#{addrstr}'")
	end
	end

WSAGetLastError and GetLastError are identical, except for their return types. Since this string interpolation allocates memory before the body of SystemError::ClassMethods#from_wsa_error calls WinError.wsa_value, the error message will always be "The operation completed successfully.".

One might argue that this is sensible behavior, in that Errno.value shares the same caveat with respect to C functions. But while functions in POSIX or the C standard library, including malloc, may write positive integers to errno regardless of whether a failure occurred, they never write 0. Indeed, if the top snippet prints neither ERROR_INVALID_PARAMETER nor ERROR_SUCCESS, then a genuine error might have happened somewhere else. ERROR_SUCCESS falls out of this natural expectation.

We could work around this by defining an appropriate Socket::Error.build_message, but I wonder if there is a better solution for the general case, other than preserving WinError.value on every single dynamic allocation.

[straight-shoota]

I'm wondering about the use of build_message for this, as proposed in #15577.
I suppose build_message is for templating customized error messages, so it might be an acceptable usage.

However, this is not directly addressing the underlying issue. It's only treating a symptom. If later changes would introduce some other allocation somewhere in between, we'd be facing the same problem again. And if we add more customized error messages, we'll need to add special build_message overloads for every single one.

A better fix would be to retrieve the errno value immediately after the failed function call and not allow the possibility of any code to interfere in between.

This could be explicit, retrieving Errno.value and passing it to the error constructor.

Perhaps another idea could be a yielding constructor method where customized error messages would be constructed in a block? This is just thinking aloud, I haven't really considered whether that would be feasible.

[HertzDevil]

A better fix would be to retrieve the errno value immediately after the failed function call and not allow the possibility of any code to interfere in between.

This requirement would be tantamount to deprecating .from_winerror and .from_wsa_error, plus .from_errno for best practices as well, since their very existence allows said string interpolation to interfere inbetween.

If these were macros, we might be able to tamper with the evaluation order, ensuring WinError.value gets called before {{ message }} is expanded. Still this only applies to a single call.

[straight-shoota]

The pattern of calling a lib fun, checking its return value and then raising an exception from errno is quite common. Maybe we could extract that into a helper which would also take care of retrieving the errno value right away?

def handle_fun_error(ret, klass)
  if ret != 0
    errno = Errno.value
    message = yield ret
    raise klass.from_os_error(message, errno)
  else
    ret
  end
end

handle_fun_error(LibFoo.foo(x), RuntimeError) { "error calling foo with #{x}" }

This could be a relatively neat solution. It completely encapsulates the error handling for lib function calls. But I'm not entirely convinced.
The current, expressive solution is some simple boilerplate, that makes it very clear what happens. This handle_fun_error hides the behaviour. It's not clear at all what this does. Maybe a better descriptive name could help, or turning this into a macro. But I doubt it can be really intuitive.

And unfortunately, there are a couple of variations on what return values indicate success. Or some specific errno's can be ignored. It doesn't seem realistic that we could encode all this additional complexity in a simple error handler method. Maybe there could be multiple ones for different use cases.
But in the end, it just replaces very simple code with an opaque abstraction. 😕

Full example: https://carc.in/#/r/hzff

[straight-shoota]

What if we move the pattern to create the message in a block to the original .from_errno methods?
Or maybe it wouldn't be a bad idea to turn .from_errno etc. into a macro. They're class methods anyway and implemented as a simple delegation. So there shouldn't be much trouble with that.

macro from_errno(message : String, **opts)
  %errno = Errno.value
  {{@type}}.from_os_error({{ message }}, %errno, {{ opts.double_splat }})
end

Full example with all variants: https://carc.in/#/r/hzfn

[HertzDevil]

+1 for turning them into macros, this should be minimally intrusive despite being a breaking change

[HertzDevil]

One problem with {{@type}} is that referring to ::File::Error from Crystal::System::File is now impossible, but ::{{@type}} would make file-private includers of SystemError unusable.

[straight-shoota]

Yes, that's a bit of an issue. However, are file-private includers of SystemError a relevant use case?