[OpenMP] Fix various alignment issues

[rorth]

When running the openmp testsuite on 32-bit SPARC, several tests FAIL apparently randomly, but always with the same kind of error:

# error: command failed with exit status: -11

The tests die with SIGBUS, as can be seen in truss output:

26461/1:            Incurred fault #5, FLTACCESS  %pc = 0x00010EAC
26461/1:              siginfo: SIGBUS BUS_ADRALN addr=0x0013D12C
26461/1:            Received signal #10, SIGBUS [default]
26461/1:              siginfo: SIGBUS BUS_ADRALN addr=0x0013D12C

i.e. the code is trying an unaligned access which cannot work on SPARC, a strict-alignment target which enforces natural alignment on access. This explains the apparent randomness of the failures: if the memory happens to be aligned appropriately, the tests work, but fail if not.

A Debug build reveals much more:

• __kmp_alloc currently aligns to sizeof(void *), which isn't enough on strict-alignment targets when the data are accessed as types requiring larger alignment. Therefore, this patch increases alignment to SizeQuant.

• 32-bit Solaris/sparc libc guarantees 8-byte alignment from malloc, so this patch adjusts SizeQuant to match.

• There's a SIGBUS in

  __kmpc_fork_teams (loc=0x112f8, argc=0, 
      microtask=0x16cc8 <__omp_offloading_ffbc020a_4b1abe_main_l9_debug__.omp_outlined>)
      at openmp/runtime/src/kmp_csupport.cpp:573
  573	  *(kmp_int64 *)(&this_thr->th.th_teams_size) = 0L;
  

  Casting to a pointer to a type requiring 64-bit alignment when that isn't guaranteed is wrong. Instead, this patch uses memset instead.

• There's another SIGBUS in

  0xfef8cb9c in __kmp_taskloop_recur (loc=0x10cb8, gtid=0, task=0x23cd00, 
      lb=0x23cd18, ub=0x23cd20, st=1, ub_glob=499, num_tasks=100, grainsize=5, 
      extras=0, last_chunk=0, tc=500, num_t_min=20, 
      codeptr_ra=0xfef8dbc8 <__kmpc_taskloop(ident_t*, int, kmp_task_t*, int, kmp_uint64*, kmp_uint64*, kmp_int64, int, int, kmp_uint64, void*)+240>, 
      task_dup=0x0)
      at openmp/runtime/src/kmp_tasking.cpp:5147
  5147	  p->st = st;
  

  p->st doesn't currently guarantee the 8-byte alignment required by kmp_int64 st. p is set in

   __taskloop_params_t *p = (__taskloop_params_t *)new_task->shareds;
  

  but shareds_offset is currently aligned to sizeof(void *) only. Increasing it to sizeof(kmp_uint64) to match its use fixes the SIGBUS.

With these fixes I get clean openmp test results on 32-bit SPARC (both Solaris and Linux), with one unrelated exception.

Tested on sparc-sun-solaris2.11, sparcv9-sun-solaris2.11, sparc-unknown-linux-gnu, sparc64-unknown-linux-gnu, i386-pc-solaris2.11, amd64-pc-solaris2.11, i686-pc-linux-gnu, and x86_64-pc-linux-gnu.

[brad0]

Do we want to add anything to the comment above this?

[rorth]

I found this is complete mess: glibc sysdeps/i386/malloc-alignment. does #define MALLOC_ALIGNMENT 16, contrary to the comment (haven't checked if this has changed historically). Besides, KMP_HAVE_QUAD is a complete misnomer: it's only defined on x86 targets, so should probably be renamed to KMP_X86_HAVE_QUAD to reflect that.

[rorth]

I've updated this comment now, removing the incorrect Linux reference.

As it turns out, malloc alignment is a total can of worms:

• In theory, the address returned should be aligned to alignof(max_align_t).
• However, implementations often differ. E.g. Solaris 32-bit libc malloc only uses 8-byte alignment. At the time this was introduced, __float128 didn't exist on x86 yet and it was considered too risky to raise this later for fear of breaking binary compatiblity. Similarly, SPARC uses 8-byte alignment, too, although one could argue that this should be 16 bytes due to the use of 128-bit long double.
• Many platforms have different malloc implementions that often differ from the one in libc.
• The actual alignment cannot be queried at build or runtime, unfortunately.

That said, it might be prudent to switch to one of memalign, posix_memalign, or _aligned_malloc. GCC's libgomp (in libgomp/alloc.c:gomp_aligned_alloc) went that route, e.g.

[shiltian]

Casting to a pointer to a type requiring 64-bit alignment when that isn't guaranteed is wrong.

kmp_teams_size_t is a struct of two kmp_int32 so its size is 64-bit, and the default alignment is its size, which would also be 64-bit. What could be wrong here?

[rorth]

Casting to a pointer to a type requiring 64-bit alignment when that isn't guaranteed is wrong.

kmp_teams_size_t is a struct of two kmp_int32 so its size is 64-bit, and the default alignment is its size, which would also be 64-bit. What could be wrong here?

No, its alignment is the alignment of the member with the strictest alignment requirement (i.e. alignof(kmp_int32), which is alignof(int), so just 4. So casting the address to a kmp_int64 * extends the alignment requirement, and this is exactly what I saw: an insn requiring 8-byte alignment accessing 4-byte-aligned data.

[brad0]

@shiltian

[shiltian]

LGTM but I'd like @jprotze @jpeyton52 to have a second look.

[mjklemm]

LGTM, @jpeyton52 please add your blessing, if you're OK with this PR.

[jprotze]

I cannot find any assignment to this variable. What is the purpose of having this as a static?

[jpeyton52]

As @jprotze notes we can get rid of this global and make it a constexpr size_t:

Suggested change

	static int alignment = SizeQuant;
	constexpr size_t alignment = SizeQuant;

It is only used once down below in the __kmp_alloc() function (line 1929).

[jpeyton52]

Rest looks good to me.

[jpeyton52]

As @jprotze notes we can get rid of this global and make it a constexpr size_t:

Suggested change

	static int alignment = SizeQuant;
	constexpr size_t alignment = SizeQuant;

It is only used once down below in the __kmp_alloc() function (line 1929).

[jprotze]

Lgtm

[rorth]

I believe we're good now?

[jpeyton52]

LGTM