Align to 4-byte boundaries for 32-bit environments#315
Conversation
| // and Timing. Since those 3 metric types behave the same way and are sampled | ||
| // with the same type they're represented by the same class. | ||
| type bufferedMetricContexts struct { | ||
| _ [4]byte // Aligin to a 4-byte boundary |
There was a problem hiding this comment.
I have a concern that this approach can move the problem, but not eliminate it.
Aggregator is heap-allocated so it starts at an 8-byte aligned base address. On 32-bit the layout of aggregator up to histograms sums to offset 64 (83 + 4 + 123), so histograms.nbContext is fine. The problem is distributions and timings, which follow histograms at offset 64 + sizeof(bufferedMetricContexts). If that size is not a multiple of 8, the nbContext field in distributions/timings falls at a 4-byte but not 8-byte aligned address.
If we add 4 bytes at the start of the struct, it moves nbContext to offset 4 within bufferedMetricContexts, but then histograms starts at base+64, histograms.nbContext is now at base+68 so not aligned.
There was a problem hiding this comment.
Thank you for your detailed comment.
You are right. I wanted to fix this panic on a 32-bit binary with Go 1.22.4:
panic: unaligned 64-bit atomic operation
The panic happened around atomic.AddUint64(&bc.nbContext, ...) .
But I now understand that my fix is not enough.
Adding 4 bytes before nbContext may only move the alignment problem to another field, as you said.
Also, some things have changed on my side since I opened this PR.
So I will debug this again in my current environment.
I will check the struct layout with GOARCH=386.
If my understanding is correct, a better fix may be to keep nbContext as the first field, and add [4]byte padding after randomLock .
Thank you again for explaining this carefully.
|
Hi @unionsep , First and foremost, thank you for reporting the problem and offering a solution! I left a comment which explains my concern. Could you please share your opinion on that? |
TL;DR
This Pull Request addresses memory alignment issues encountered in Go's 32-bit build environment.
Longer version
I need to run DataDog in a Go 32-bit build environment. While using DataDog/dd-trace-go.v1, I encountered the following panic error in datadog-go:
(Reference: BufferedMetricContext.go Line 53)
In a 32-bit build environment, it's sometimes not possible to align 64-bit data types correctly to an 8-byte boundary. To address this, I aligned to a 4-byte boundary using an underscore variable. This change allowed DataDog to operate correctly in the 32-bit build environment.
Although this is a rare case, I would appreciate it if you could consider incorporating this fix.