[Issue]: Undefined behavior (misaligned allocation) for Semaphores in Thread

[LunNova]

Problem Description

/build/source/rocclr/thread/thread.cpp:39:18: runtime error: constructor call on misaligned address 0x506000010be0 for type 'Semaphore', which requires 64 byte alignment
0x506000010be0: note: pointer points here
 00 00 00 00  be be be be be be be be  be be be be be be be be  be be be be be be be be  be be be be
              ^ 
    #0 0x7fff868239f7 in create /build/source/rocclr/thread/thread.cpp:39
    #1 0x7fff86823416 in Thread /build/source/rocclr/thread/thread.cpp:52
    #2 0x7fff8682303c in HostThread /build/source/rocclr/thread/thread.cpp:32
    #3 0x7fff86824905 in amd::Thread::init() (/nix/store/2yvyn8dgbwxm0fihv1zqc6z86fwr1dk5-rocm-hip-libraries-meta/lib/libamdhip64.so.6+0xc24905)

The three Semaphore instances created with operator new in Thread are created with a misaligned address, causing a undefined behavior error on library init if built with -fsanitize=undefined

clr/rocclr/thread/thread.cpp

Lines 39 to 41 in b8ba4cc

	created_ = new Semaphore();
	lock_ = new Semaphore();
	suspend_ = new Semaphore();

I'm not sure why. Semaphore is marked alignas(64) and if I understand right operator new should handle this as of C++17.

Operating System

NixOS

CPU

7773

GPU

MI210

ROCm Version

ROCm 6.3.0

ROCm Component

clr

Steps to Reproduce

Build CLR with -fsanitize=undefined, dlopen libamdhip64

[LunNova]

I think this is because HeapObject which Semaphore extends from explicitly defines an operator new() impl that isn't alignment aware.

clr/rocclr/os/alloc.cpp

Lines 80 to 82 in b8ba4cc

	void* HeapObject::operator new(size_t size) { return malloc(size); }
	void HeapObject::operator delete(void* obj) { free(obj); }

C++17 defines operator new with a different signature that allows alignment. If HeapObject implemented that I expect the Semaphore instances would be aligned correctly.

void* operator new[]( std::size_t count, std::align_val_t al );

It might be best to remove the operator new impls for HeapObject instead. The default already is to use malloc so explicitly implementing via malloc seems like redundant code.

[ppanchad-amd]

Hi @LunNova. Internal ticket has been created to fix this issue. Thanks!

[tcgu-amd]

Hi @LunNova, after some investigation, we have verified that the new operator override is indeed legacy code and we have raised a patch in our internal pipeline. I will close this issue for now since there's no further actionable items, but please feel free to post follow-ups regarding the status of the patch. Thanks!

[LunNova]

Can you publish the patch somewhere externally accessible so I can verify it?

[tcgu-amd]

Can you publish the patch somewhere externally accessible so I can verify it?

Sure, I can try to duplicate the PR to external.

[tcgu-amd]

Here you go #128.

A bit of explanation: we realized that due to how new is being handled, the alignment requirement has never really been satisfied at runtime. Instead of removing the "new" definition from HeapObject and risk things breaking down the line, we have decided to just remove the the memory alignment requirement after discussing with the original author of the code.

Thanks!

[LunNova]

Confirmed that the error is gone on 21d7645

🎉