Skip to content

Conversation

HertzDevil
Copy link
Contributor

Tested using 18.1.0-rc1 which had no issues.

Note that LLVM's release process has changed; x.0.0 is now reserved for development branches, and the first stable release for each major version is now x.1.0, similar to GCC.

This PR also moves Windows CI to 18.1.0-rc1 directly. Linux CI will have to wait as we still depend on their volunteers' binary packages (they now have a nice note telling people to wait).

@HertzDevil HertzDevil added kind:feature platform:windows Windows support based on the MSVC toolchain / Win32 API topic:stdlib:llvm topic:infrastructure/ci labels Jan 30, 2024
@HertzDevil HertzDevil marked this pull request as draft January 30, 2024 14:49
@HertzDevil
Copy link
Contributor Author

HertzDevil commented Jan 30, 2024

Looks like this had issues after all and something about Socket::IPAddress is failing:

require "socket"

x = Socket::IPAddress.new("::1", 1)
x == x # Invalid memory access (signal 11) at address 0x0

This reduces to:

struct In6Addr
  @__in6_u = 0_i128

  def ==(other : self)
    @__in6_u == other.@__in6_u
  end

  def ==(other)
    false
  end
end

struct Int32
  def ==(other : In6Addr)
    false
  end
end

addr = In6Addr.new.as(Int32 | In6Addr)
addr == addr

This part of LLVM 18 looks suspicious:

The i128 type now matches GCC and clang's __int128 type. This mainly benefits external projects such as Rust which aim to be binary compatible with C, but also fixes code generation where LLVM already assumed that the type matched and called into libgcc helper functions.

Note that alignof(In6Addr) == 16 but alignof(Int32 | In6Addr) == 8. The same is also reproducible using Tuple(Int128) instead of In6Addr. So maybe this is related to #13609?

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Jan 30, 2024

Digging a bit further, the above reduced snippet crashes at something like this, indeed an unaligned 128-bit read: (the SIGSEGV is indistinguishable from a null pointer exception since it appears x86-64 doesn't hand out the actual failing address to the signal handler)

Process 69176 stopped
* thread #1, name = 'test', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
    frame #0: 0x000055555555542c test`==(self=(__in6_u = 2596148429080290042522275961362632), other=(__in6_u = 140737488345184)) at test.cr:64:3
   61   struct In6Addr
   62     @__in6_u = 0_i128
   63  
-> 64     def ==(other : self)
   65       @__in6_u == other.@__in6_u
   66     end
   67  
(lldb) di -p
test`:
->  0x55555555542c <+60>: movaps (%rcx), %xmm0
    0x55555555542f <+63>: movaps (%rax), %xmm1
    0x555555555432 <+66>: pcmpeqb %xmm1, %xmm0
    0x555555555436 <+70>: pmovmskb %xmm0, %eax
(lldb) register read rcx xmm0
     rcx = 0x00007fffffffd8c8
    xmm0 = {0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
(lldb) v
(In6Addr) self = (__in6_u = 2596148429080290042522275961362632)
(In6Addr) other = (__in6_u = 140737488345184)

(Int32 | In6Addr) being underaligned means other isn't even passed correctly, as 128-bit integers do not fit into ordinary registers. For comparison, the assembly for In6Addr#==(In6Addr) looks like this in LLVM 18 (this uses --no-debug rather than --debug, so it is slightly different):

"*In6Addr#==<In6Addr>:Bool":
	.cfi_startproc
	movq	%rdi, -48(%rsp)
	movq	%rdx, -40(%rsp)
	movq	%rsi, -32(%rsp)
	movq	-48(%rsp), %rax
	movq	-40(%rsp), %rcx
	movq	-32(%rsp), %rdx
	movq	%rdx, -24(%rsp)
	movq	%rcx, -16(%rsp)
	movaps	(%rax), %xmm0 ; crashes here
	movaps	-24(%rsp), %xmm1
	pcmpeqb	%xmm1, %xmm0
	pmovmskb	%xmm0, %eax
	subl	$65535, %eax
	sete	%al
	retq

Same method in LLVM 15:

"*In6Addr#==<In6Addr>:Bool":
	.cfi_startproc
	movq	%rdi, -40(%rsp)
	movq	%rdx, -32(%rsp)
	movq	%rsi, -24(%rsp)
	movq	-40(%rsp), %rax
	movq	-32(%rsp), %rcx
	movq	-24(%rsp), %rdx
	movq	%rdx, -16(%rsp)
	movq	%rcx, -8(%rsp)
	movups	(%rax), %xmm0 ; ok, movups can read from underaligned addresses
	movups	-16(%rsp), %xmm1
	pcmpeqb	%xmm1, %xmm0
	pmovmskb	%xmm0, %eax
	subl	$65535, %eax
	sete	%al
	retq

@HertzDevil HertzDevil marked this pull request as ready for review February 2, 2024 22:09
@straight-shoota straight-shoota added this to the 1.12.0 milestone Feb 2, 2024
@straight-shoota straight-shoota merged commit c67883c into crystal-lang:master Feb 4, 2024
@Blacksmoke16
Copy link
Member

Blacksmoke16 commented Feb 5, 2024

I haven't really looked into it too much yet, but Athena's nightly windows CI job fails now, while the latest release passes. I'll see about reproducing it and will go from there.

EDIT: Created #14285

@HertzDevil HertzDevil deleted the feature/llvm-18.1 branch February 8, 2024 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:feature platform:windows Windows support based on the MSVC toolchain / Win32 API topic:infrastructure/ci topic:stdlib:llvm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants