aix: add required changes to build with clang [v8/v8 : main]
Toon Verwaest (Gerrit)
Toon Verwaest added 4 comments![]( "Open in Gerrit")
__ bind(&no_stack_change);This label could move into the branch above?
struct __attribute__((packed)) EffectDimensions {Why is this necessary?
void* function_desc[3];Is there no way to achieve this without handwritten assembly?
__thread bool TrapHandlerGuard::is_active_ = 0;Why is this needed?
Related details
- Abdirahim
- Mark Seaborn
- Matthias Liedtke
- Michael Achenbach
- Milad Farazmand
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Korinne Adler (Gerrit)
Korinne Adler added 2 comments![]( "Open in Gerrit")
struct __attribute__((packed)) EffectDimensions {Why is this necessary?
On AIX, Clang (like XLC) promotes bit fields to unsigned int unless packed. This breaks the assertions that the size of these structs are 1 byte large.
void* function_desc[3];Is there no way to achieve this without handwritten assembly?
We tried multiple ways to do this without, but Clang kept optimizing the descriptor setup away since casting to it is UB. Doing it directly in assembly assures that we get the behavior we need without the optimizer getting in our way.
Toon Verwaest (Gerrit)
Toon Verwaest added 2 comments![]( "Open in Gerrit")
struct __attribute__((packed)) EffectDimensions {Korinne AdlerWhy is this necessary?
On AIX, Clang (like XLC) promotes bit fields to unsigned int unless packed. This breaks the assertions that the size of these structs are 1 byte large.
I suppose we could use bitfields or maybe compile v8 with `-fpack-struct` but I guess this is less invasive and more likely to just work.
void* function_desc[3];Korinne AdlerIs there no way to achieve this without handwritten assembly?
We tried multiple ways to do this without, but Clang kept optimizing the descriptor setup away since casting to it is UB. Doing it directly in assembly assures that we get the behavior we need without the optimizer getting in our way.
- Abdirahim
- Korinne Adler
Abdirahim (Gerrit)
Abdirahim added 1 comment![]( "Open in Gerrit")
__thread bool TrapHandlerGuard::is_active_ = 0;AbdirahimWhy is this needed?
This is needed because clang complains that is_active is not initialized. on AIX is_active is a __thread.
Related details
- Korinne Adler
- Mark Seaborn
- Matthias Liedtke
- Milad Farazmand
- Toon Verwaest
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abdirahim (Gerrit)
Abdirahim added 1 comment![]( "Open in Gerrit")
__ bind(&no_stack_change);This label could move into the branch above?
I think we might be able to. @mfar...@ibm.com do you think this would be ok?
Milad Farazmand (Gerrit)
Milad Farazmand added 1 comment![]( "Open in Gerrit")
__ bind(&no_stack_change);AbdirahimThis label could move into the branch above?
I think we might be able to. @mfar...@ibm.com do you think this would be ok?
- Abdirahim
- Korinne Adler
- Mark Seaborn
- Matthias Liedtke
- Toon Verwaest
Toon Verwaest (Gerrit)
Toon Verwaest added 2 comments![]( "Open in Gerrit")
"li 0, 0\n\t"I don't 100% understand what this code does; but gemini told me that this clobbers r0 which isn't on the clobber list?
__thread bool TrapHandlerGuard::is_active_ = 0;AbdirahimWhy is this needed?
This is needed because clang complains that is_active is not initialized. on AIX is_active is a __thread.
Mmh ok weird. Sounds like this might be a linker issue; while `thread_local` should actually be supported by clang?
Related details
- Abdirahim
- Korinne Adler
- Mark Seaborn
- Matthias Liedtke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Korinne Adler (Gerrit)
Korinne Adler added 3 comments![]( "Open in Gerrit")
"li 0, 0\n\t"I don't 100% understand what this code does; but gemini told me that this clobbers r0 which isn't on the clobber list?
It does indeed clobber r0, which is why it _is_ in the clobber list (along with memory).
The code is equivalent to the C++ code it replaces, initializing `function_desc` and assigning the address of the descriptor to the `Signature* fn`.
Line by line explanation:
```
std %1, 0(%2) // function_desc[0] = fn_ptr_
li 0, 0 // set r0 = 0 for use in the next two instructions
std 0, 8(%2) // function_desc[1] = 0
std 0, 16(%2) // function_desc[2] = 0
mr %0, %2 // fn = function_desc
```
__thread bool TrapHandlerGuard::is_active_ = 0;AbdirahimWhy is this needed?
Toon VerwaestThis is needed because clang complains that is_active is not initialized. on AIX is_active is a __thread.
Mmh ok weird. Sounds like this might be a linker issue; while `thread_local` should actually be supported by clang?
In trap-handler-internal.h, `is_active_` is declared as a `__thread` on AIX and links to a GCC bug report to explain why. However, the definition of the variable here still used `thread_local`, which while apparently acceptable on GCC gave a compile error on Clang.
We reached out to the AIX Clang devs to see the recommended way to fix this along with some other TLS-related questions. Hubert Tong (who commented in the GCC bug mentioned previously) told us that using `thread_local` here should be fine, but that it incurs extra overhead over using `__thread`. Since we still need to support GCC for now, it's easier to leave it consistent for AIX between GCC and Clang. Once we drop GCC we should be able to remove the ifdefs.
__thread bool TrapHandlerGuard::is_active_ = 0;AbdirahimWhy is this needed?
Toon VerwaestThis is needed because clang complains that is_active is not initialized. on AIX is_active is a __thread.
Mmh ok weird. Sounds like this might be a linker issue; while `thread_local` should actually be supported by clang?
In trap-handler-internal.h, is_active_ is defined as a __thread on AIX and links to a GCC bug report to explain why. However, the definition of the variable here still uses thread_local, which while apparently acceptable on GCC gave a compile error on Clang.
We reached out to the AIX Clang devs to see the recommended way to fix this along with some other TLS-related questions. Hubert Tong (who commented in the GCC bug mentioned previously) told us that using thread_local here should be fine, but that it incurs extra overhead over using __thread. Since we still need to support GCC for now, it's easier to leave it consistent for AIX between GCC and Clang. Once we drop GCC we should be able to remove the ifdefs.
Related details
- Abdirahim
- Mark Seaborn
- Matthias Liedtke