8 ms·

We found a bug in Go's ARM64 compiler

835 points by jgrahamc 1y ago

alberth 1y ago
I thought Cloudflare was 100% Rust, and x86 (EPYC) these days. Interesting to hear Go & ARM in use.
- steveklabnik 1y ago
  Cloudflare has long kept Arm builds of everything even when they deployed to x86 only, to make it easy to switch when it made sense. And yeah, a lot of Rust but also a lot of Go.
- surajrmal 1y ago
  I doubt any company is mono language at that scale. Using ARM usually makes sense for s lot of horizontal scaling workloads so it's also not that surprising.
javierhonduco 1y ago
Really enjoyed reading this. Thanks for writing it!
Neywiny 1y ago
That's an incredible find and once I saw the assembly I was right along with them on the debug path. Interestingly it doesn't need to be assembly for this to work, it's just that that's where the split was. The IR could've done it, it just doesn't for very good reasons. So another win for being able to read arm assembly. Unsure if this would be another way to do it but to save an instruction at the cost of a memory access you could push then pop the stack size maybe? Since presumably you're doing that pair of moves on function entry and exit. I'm not really sure what the garbage collector is looking for so maybe that doesn't work, but I'd be interested to hear some takes on it
- titzer 1y ago
  I think the right fix is that the compiler should, e.g. load the constant into a register using two moves and then emit a single add. It's one more instruction, but then the adjustment is atomic (i.e. a single instruction). Another option is to do the arithmetic in a temp register and then move it back.
- bloak 1y ago
  > So another win for being able to read arm assembly. Yes, though that weird stuff with dollars in it is not normal AArch64 assembly! The article could have mentioned the "stack moves once" rule.
  - pjmlp 1y ago
    It is due to the Plan 9 Assembly dialect most likely, because it wasn't enough that we already have differences between AT&T and Intel. https://go.dev/doc/asm https://go.dev/doc/asm Still, I find great that Go got back the 1990's tradition that compiled languages have an assembler as part of their tooling, regardless of the syntax.
  - Neywiny 1y ago
    I've never heard of that rule (though tbh I'm not allocating > 64KB of stack when I'm in assembly) and it seems Google hasn't either. While I'm sure it makes sense, I don't think I've ever seen that be enforced. At least in C/C++. Maybe it makes more sense for these stack inspecting garbage collectors but I've also heard of ones that just scan the stack without unwinding anything. I did a test asking Google's AI to generate a complicated C function, put it in godbolt, and there's plenty of push push push push ..... Pop Pop Pop Pop going on
    - rcxdude 1y ago
      Did you compile with optimisations? I think GCC will do a bunch of activity on the stack with -O0, but it'll generally coalesce everything into one push/pop per function with optimisations (not because of any rule, but just because it's faster). alloca and other dynamic stack allocation may break this, but normal variables should in pretty much all just get turned into one block on the stack (with appropriate re-use of space if variable lifetimes don't overlap)
Agingcoder 1y ago
Excellent article as always from the cloudflare blog - engineering without magic infrastructure and ml. One day I will apply ! Compiler bugs are actually quite common ( I used to find several a year in gcc ), but as the author says, some of them only appear when you work at a very large scale, and most people never dive that far.
- jgrahamc 1y ago
  What's stopping you applying today?
  - nevon 1y ago
    Similar to the previous commenter, every time I read a blog post from Cloudflare I end up checking the careers page thinking "this is exactly the kind of work I'd like to be doing". Sadly no openings in my country. I'll keep checking!
    - moomoo11 1y ago
      Pretty sure location is not a factor for these companies. You should apply anyway. I’ve worked with people living in active war zones. If you have the skills, they have the coin. They won’t hire some react guy in X country but someone who can find compiler bugs and save them XX+ million dollars a year? Heck yeah.
      - Degorath 1y ago
        Unfortunately, in 95% cases location IS a factor with bigger companies. I'm in a similar position where I'd like to do something a lot more interesting, but intersection between where the interesting companies have offices and where I'd be willing to live do not really overlap enough justify rooting up my life. (Unless we're talking about "too good to ignore", that's a different story.)
        moomoo11 1y ago
dreamcompiler 1y ago
Always adjust your stack pointer atomically, kids.
- whizzter 1y ago
  I guess those that wrote the preemption were on X86 where this doesn't happen thanks to variable length instructions being able to hold the constant and thus relied on the code-gen to do it atomically, then the ARM port had an automatic "split" from a higher level to make things "easy" thus giving us this bug. Nobodys fault really, but bad results ensued.
  - Sesse__ 1y ago
    > Nobodys fault really, but bad results ensued. Uh, the fault is entirely in writing an assembler _that is not an assembler_, but rather something that is _almost_ like one but then 1% like an IR instead. It's an unforced error.
    - wbl 1y ago
      Assemblers used to do a ton of stuff back in the day
      - anyfoo 1y ago
        Oh yeah. S/360 assembly almost looks like a high level language sometimes. In MVS, functions of the OS and standard libraries (or its equivalent) were implemented as elaborate macros, with their own invocation syntax, whereas nowadays you'd expect a function that you'd call (dynamically linked or not), with parameters passed in registers. At least in the 90s, there were actually macro assemblers that supported OOP programming in assembly. Borland Turbo Assembler 5.0 comes to mind, if was kind of fun.
        pjmlp 1y ago
        Those are still around if you go for Assemblers with background in PC culture like NASM, YASM, MASM (still part of MSVC). By the way Embarcaredo still has Turbo Assembler. https://docwiki.embarcadero.com/RADStudio/Athens/en/Turbo_Assembler_Options https://docwiki.embarcadero.com/RADStudio/Athens/en/Turbo_As... Now a thing of the past, but Assemblers for game consoles were also quite powerfull in their macro capabilities. I never liked the UNIX Assembly culture, because naturally as soon as C became a thing, they became the bare minimum required to assemble the generated Assembly out of the C compiler, as another step into the compilation pipeline. All the niceties of macro assemblers came through the other platforms, like being able to use NASM instead of the platform assembler, not even GNU AS nor clang are that great in their abilities as Assemblers beyond the basic stuff.
pengaru 1y ago
For the impatient, here's the fix: https://github.com/golang/go/commit/f7cc61e7d7f77521e073137c6045ba73f66ef902 https://github.com/golang/go/commit/f7cc61e7d7f77521e073137c...
- cmckn 1y ago
  I noticed this when reviewing the linked issue: https://github.com/golang/go/issues/73259#issuecomment-3100443997 https://github.com/golang/go/issues/73259#issuecomment-31004... Does the Go team have a natural language bot or is this just comment.contains(“backport”) type stuff?
  - kbolino 1y ago
    The latter: https://github.com/golang/build/blob/master/cmd/gopherbot/gopherbot.go https://github.com/golang/build/blob/master/cmd/gopherbot/go... (found via https://go.dev/wiki/gopherbot https://go.dev/wiki/gopherbot)
    - 9rx 1y ago
      Although also the former (gabyhelp): https://github.com/golang/oscar/tree/master/internal/gaby https://github.com/golang/oscar/tree/master/internal/gaby
    - etra0 1y ago
      Kinda funny that it requires both "please" and "backport" for it to be considered haha.
- chavi2 1y ago
  One thing I worry about, probably unnecessarily, is anything with a sense of urgency. HEY GUYS WE JUST FOUND A GOLANG COMPILER BUG AND FATAL PANICS! Everyone is like “Hmm. I need to fix this now.” So, 99% probability it’s what it is. 1% it’s some secret defensive thing because there was a bad stupid zero day someone would get fired over or that could leave the world in shambles if uncovered, or maybe something else needed to be swept under the rug, or maybe someone wants to distract while they introduce a new vulnerability. I don’t think this with CVEs, but when someone’s like “install this patch everybody!” the dim red light flickers on.
riobard 1y ago
What ARM64 machines are you using and what are they used for? Last year you were announcing Gen 12 servers on AMD EPYC (https://blog.cloudflare.com/gen-12-servers/ https://blog.cloudflare.com/gen-12-servers/), but IIRC there weren’t any mentions of ARM64. But now it seems you’re running ARM64 in full production.
- zamadatix 1y ago
  I'm not Cloudflare, I just read their blog too much. As they hint in the article when mentioning secure boot, they've been deploying Ampere in parallel to AMD for several years now. Purpose wise it seems to be Edge related for efficiency reasons, but maybe they use them for other things too. You can read some more here https://blog.cloudflare.com/designing-edge-servers-with-arm-cpus/ https://blog.cloudflare.com/designing-edge-servers-with-arm-... and here https://blog.cloudflare.com/arms-race-ampere-altra-takes-on-aws-graviton2/ https://blog.cloudflare.com/arms-race-ampere-altra-takes-on-... along with the original evaluation of Qualcomm here https://blog.cloudflare.com/arm-takes-wing/ https://blog.cloudflare.com/arm-takes-wing/
  - riobard 1y ago
    Yeah but those are pretty dated. I was under the impression those old Ampere servers are not efficient compared to modern EPYC anymore. So I’m wondering what their current generation of arm64 servers look like :p
- EE84M3i 1y ago
  I seem to recall Cloudflare hosts their some of their non-edge compute on public clouds? Like control plane stuff. Could be that.
gok 1y ago
The real lesson here should be that doing crazy shit like swizzling the program counter in a signal handler and writing your own assembler is not a good idea.
- blinkingled 1y ago
  This^. Keith W on Dtrace blog said it a decade ago https://wesolows.dtrace.org/2014/12/29/golang-is-trash/ https://wesolows.dtrace.org/2014/12/29/golang-is-trash/ I like Go but I don't really like their NIH / replace everything with our stuff stance - esp on system tools like assemblers and linkers.
- wat10000 1y ago
  The general wisdom is that you shouldn't do this stuff yourself, and you should instead rely on tried and tested implementations. But sometimes you're the one who provides the tried and tested implementations. Implementing a compiled language is often one of those times.
- themafia 1y ago
  Neither of those are "crazy shit." It's just complex because the environment offers specific features like automatic GC with async preemption in a compiled language which pretty much requires it. Complex engineering isn't something to be avoided by default.
  - Diggsey 1y ago
    Agree, but I think there is a point to be made here: Go as a language has more subtle runtime invariants that must be upheld compared to other languages, and this has led to a relatively large number of really nasty bugs (eg. there have also been several bugs relating to native function calling due to stack space issues and calling convention differences). By "nasty" I mean ones that are really hard to track down if you don't have the resources that a company like CF does. To me this points to a lack of verification, testing, and most importantly awareness of the invariants that are relied on. If the GC relies on the stack pointer being valid at all times, then the IR needs a way to guarantee that modifications to it are not split into multiple instructions during lowering. It means that there should be explicit testing of each kind of stack layout, and tests that look at the real generated code and step through it instruction by instruction to verify that these invariants are never broken...
renewiltord 1y ago
Great technical blog. Good pathway for narrative, tight examples, description so clear it makes me feel smarter than I am because so easy to follow though the last time I even read assembly seriously was x86 years ago. Also, fulfills the marketing objective because I cannot help but think that this team is a bunch of hotshots who have the skill to do this on demand and the quality discipline to chase down rare issues. I assume these are Ampere Altra? I was considering some of those for web servers to fill out my rack (more space than power) but ended up just going higher on power and using Epyc.
wat10000 1y ago
I would have thought that unwinding would use the frame pointer and this wouldn't be a problem.
- mperham 1y ago
  The frame pointer was updated non-atomically in two asm ops. An async interruption between the two ops would lead to a corrupt frame pointer.
  - wat10000 1y ago
    So it was. The article never mentions the frame pointer and I'm familiar with compilers that load the saved value from the stack in the epilog, rather than adjusting it arithmetically. But they do have an assembly listing showing the two-step arithmetic adjustment for both the stack pointer and frame pointer. But I'm not sure that matters, because the unwind code they show uses the stack pointer rather than the frame pointer anyway.
berz01 1y ago
[flagged]
mperham 1y ago
Did they ever explain why netlink was involved? Or was that a red herring?
- Sesse__ 1y ago
  The stack in that specific function was big enough to trigger the bug.
- drob518 1y ago
  Seemed like a red herring. They were able to reproduce it without any libraries. Might have just been net link forcing the stacks to a certain size and that made the bug visible.
- syncsynchalt 1y ago
  The netlink function uses a larger stack than most. Their repro case required a stack adjustment larger than 1<<12 (4kiB).
brcmthrowaway 1y ago
I don't get it, how were the machine threads being stopped in thr middle of two instructions? This is baremetal, right?
- adgjlsfhk1 1y ago
  go uses interrupts for GC notifications
- purplesyringa 1y ago
  Signals.
  - ahoka 1y ago
    That's why the old advice was not to use signals and threads together, if you can avoid it.
lordnacho 1y ago
> This was a very fun problem to debug. I'm sure it was a relief to find a thorough solution that addressed the root cause. But it doesn't seem plausible that it was fun while it was unexplained. When I have this kind of bug it eats my whole attention. Something this deep is especially frustrating. Nobody suspects the standard library or the compiler. Devs have been taught from a young age that it's always you, not the tools you were given, and that's generally true. One time, I actually did find a standard library bug. I ended up taking apart absolutely everything on my side, because of course the last hypothesis you test is that the pieces you have from the SDK are broken. So a huge amount of time is spent chasing the wrong lead when it actually is a fundamental problem. On top of this, the thing is a race condition, so you can't even reliably reproduce it. You think it's gone like they did initially, and then it's back. Like cancer.
- akerl_ 1y ago
  It feels like this comment was almost a purely additive anecdote of your own experience with a similar kind of issue, but you've spoiled it by deciding to tell the author that they're incorrect about how they felt during the process? Maybe different people find different things fun.
  - lordnacho 1y ago
    Not saying he's wrong, sometimes the word "fun" connotes something slightly different what what it literally means. "Satisfying" is something I'd use for the end state. Maybe "challenging" for the intermediate state. But while you're in a high-pressure situation that you don't understand, that is rarely "fun" in the literal sense. You wouldn't pay to be given compiler race condition bugs, right?
    - akerl_ 1y ago
      Maybe stop digging here and just let it be fun for the author?
    - a10c 1y ago
      > Not saying he's wrong https://heinen.dev/ https://heinen.dev/ - I’m Thea “Teddy” Heinen (she/her or they/them)!
yalok 1y ago
Classic problem of non-atomic stack pointer modification. Used to have a lot of fun with those 3 decades ago.
quotemstr 1y ago
This problem strikes me more as a debuginfo generation bug than a "compiler" bug. > After this change, stacks larger than 1<<12 will build the offset in a temporary register and then add that to rsp in a single, indivisible opcode. A goroutine can be preempted before or after the stack pointer modification, but never during. This means that the stack pointer is always valid and there is no race condition. Seems silly to pessimize the runtime, even slightly, to account for the partial register construction. DWARF bytecode ought to be powerful enough to express the calculations needed for restoring the true stack pointer if we're between immediate adjustments.
- sauercrowd 1y ago
  > This problem strikes me more as a debuginfo generation bug than a "compiler" bug. But isn't that the same thing here? The bug occurred in their production workflows, not in some specific debug builds, so with that seems pretty reasonable to call it a compiler bug?
  - quotemstr 1y ago
    Thanks. I think of unwinder information as debuginfo even though, as you point out, it's used outside of debugging contexts all the time. :-) As for the actual bug: Unless you're unwinding the stack by walking the linked list of frames threaded through the frame pointer, then each time you unwind a level of the stack, you need to consult a table keyed on instruction pointer to look up how to compute the register contents of the previous frame based on register content of the current frame. One of the registers you can compute this way is the previous frame's stack pointer. I haven't looked in depth at what the Go runtime is doing exactly, but at a glance, I don't see mention of frame pointers in the linked article, so I'm guessing Go uses the SP-and-unwind-table approach? If so, the real bug here is that the table didn't have separate entries for the two ADDs and so gave incorrect reconstruction instructions for one of them. If, however, frame pointers are a load-bearing part of the Go runtime, and that runtime failed to update frame pointer (not just the stack pointer) in the contractually mandatory manner, well, that's a codegen bug and needs a codegen fix. I guess I just don't like, as a matter of philosophy if not practical engineering, having frame pointers at all. Without the frame pointer, the program already contains all the information you need to unwind, at no runtime cost --- you pay for table lookups only when you unwind, not all the time, on straight-line code. The purist in me doesn't like burning a register for debugging, but you have to use the right tool for the job I guess.
pfdietz 1y ago
I see something like this and I wonder "what testing methodology would have found this?" It has to be general, not something that would involve knowing what the bug was ahead of time.
- syncsynchalt 1y ago
  When your scale is large enough, you move to "what monitoring methodology will find this?" When you're doing enough transactions you start to see a noise floor of e.g. bit flips from cosmic rays, and looking for issues involves correlating/categorizing possible software failures and distinguishing them from the misbehavior of hardware.
  - pfdietz 1y ago
    I meant, what testing methodology could the compiler writers have used, so it was caught before it went to users. The feedback loop here should be: novel bug comes in ==> determine how existing testing was deficient ==> modify the testing in a general way that would have found this bug ==> run these modified tests in the background to see if anything similar was missed. Bugs should be used as indicators that regions (as large as possible) of bug space have been inadequately covered.
MarkSweep 1y ago
I wonder if Go had a mode where you make it single step every instruction and trigger a GC interrupt on every opcode. That would make it easier to find these kinds of bugs.
- defleopold 1y ago
  [flagged]
neuroelectron 1y ago
I've seen only one race condition in my career and it always surprises me how it is even found.
Vipsy 1y ago
One thing that often gets missed is how hard it is to even suspect the compiler as the root cause. Most engineers waste hours chasing bugs in their own code because we’re trained to trust our tools. This mindset alone can make these rare compiler bugs much trickier to find.
- kmarc 1y ago
  There are certain professions where the compilation process is (ab)used to optimize to a point where these bugs seemingly surface more often. In the HFT sphere i haven't talked to a company that hasn't reported (bragged about finding) a super weird gcc/clang bug. Well, also, at my last job we used a snapshot version of the compiler, bc... Any nanoseconds matters.
  - hshdhdhehd 1y ago
    In HFT might you keep the bug fix secret so other HFTs cant benefit from it.
    - kmarc 1y ago
      I saw both. One of the top firms wanted that, another I worked at we did report (of course with a scratched minimal reproducible example) The thing is, it's quite unlikely that your competitor hits the exact same bug. The cost of us having to keep upstream patched, tested isn't justified. Also in HFT world there are some very similar patterns across competing companies, yet, we just saw TernFS coming out from XTX, with not much fear of competitors benefiting from it more than they do.
- pjmlp 1y ago
  In the early PC days we suspected them a lot given how manually writting Assembly was still much better, in many cases. I found out a bug on Turbo Pascal 6, where if you declare a variable with the same name as the function name, then the result was random garbage. For those that don't know Pascal, the function name has to be assigned for the result value, so if a local variable with the same name is possible, then you cannot set the return value. Something like this https://godbolt.org/z/s6srhTW66 https://godbolt.org/z/s6srhTW66 (* In Turbo Pascal 6 this would compile *) function Square(num: Integer): Integer; var Square: Integer; begin Square := num * num; (* Here the local variable gets used instead *) end;
Bengalilol 1y ago
I always appreciate articles like this, where you can clearly see the engineer’s way of thinking. I was just puzzled by the middle part of the article, where they start investigating their code but seem to overlook the fact that it only happens on ARM64. Still, I understand that it’s professional to proceed step by step logically. Great article, it was a pleasure reading it!
- mixedbit 1y ago
  Hard to reproduce bugs often depend on an order of events or timing. Different architecture can trigger different order of execution, but this doesn't mean the bug is not in the application.
me2too 1y ago
Great write-up
anthk 1y ago
I miss the Delve debugger for OpenBSD 386 BTW.
bradley13 1y ago
I find it interesting, how rare it has become to find s compiler bug. For me, at least, it used to be a regular event. Even Java, as widespread as it is, I have made half-a-dozen reports. None in the last several years, though. Better testing? The sheer scale of software being produced?
- lou1306 1y ago
  Linus's law [1]? When it comes to compilers for mainstream languages, the userbases are so large that they will explore a surprisingly large portion of the compiler's state space. But definitely, better engineering and QA practices must also help here. [1] https://en.wikipedia.org/wiki/Linus%27s_law https://en.wikipedia.org/wiki/Linus%27s_law
wy1981 1y ago
Great find and writeup. As an aside, this is the type of a problem that I think model checkers can't help with. You can write perfect and complicated TLA+/Lean/FizzBee models and even if somehow these models can generate code for you from your correct models you can still run into bugs like these due to platform/compiler/language issues. But, thankfully, such bugs are rare.
- jraph 1y ago
  Yep. Model checking is for checking that your design is sound, basically, not at all the implementation. For the implementation, you can use certified compilers like CompCert [1], but: - you still have to show your code is correct - there are still parts of CompCert that are not certified [1] https://compcert.org/ https://compcert.org/
maguro_01 1y ago
An x86-64 Windows 11 machine trying to access a previously available Website now always produces a Cloudflare "obsolete protocol" error on the ordinary attempt. Al browsers get the same error. Did your fix break something?
- maguro_01 1y ago
  Sorry, all browsers.

835 points by jgrahamc 1y ago

alberth 1y ago

I thought Cloudflare was 100% Rust, and x86 (EPYC) these days. Interesting to hear Go & ARM in use.

steveklabnik 1y ago
Cloudflare has long kept Arm builds of everything even when they deployed to x86 only, to make it easy to switch when it made sense. And yeah, a lot of Rust but also a lot of Go.
surajrmal 1y ago
I doubt any company is mono language at that scale. Using ARM usually makes sense for s lot of horizontal scaling workloads so it's also not that surprising.

javierhonduco 1y ago

Really enjoyed reading this. Thanks for writing it!

Neywiny 1y ago

That's an incredible find and once I saw the assembly I was right along with them on the debug path. Interestingly it doesn't need to be assembly for this to work, it's just that that's where the split was. The IR could've done it, it just doesn't for very good reasons. So another win for being able to read arm assembly. Unsure if this would be another way to do it but to save an instruction at the cost of a memory access you could push then pop the stack size maybe? Since presumably you're doing that pair of moves on function entry and exit. I'm not really sure what the garbage collector is looking for so maybe that doesn't work, but I'd be interested to hear some takes on it

titzer 1y ago
I think the right fix is that the compiler should, e.g. load the constant into a register using two moves and then emit a single add. It's one more instruction, but then the adjustment is atomic (i.e. a single instruction). Another option is to do the arithmetic in a temp register and then move it back.
bloak 1y ago
> So another win for being able to read arm assembly. Yes, though that weird stuff with dollars in it is not normal AArch64 assembly! The article could have mentioned the "stack moves once" rule.
- pjmlp 1y ago
  It is due to the Plan 9 Assembly dialect most likely, because it wasn't enough that we already have differences between AT&T and Intel. https://go.dev/doc/asm https://go.dev/doc/asm Still, I find great that Go got back the 1990's tradition that compiled languages have an assembler as part of their tooling, regardless of the syntax.
- Neywiny 1y ago
  I've never heard of that rule (though tbh I'm not allocating > 64KB of stack when I'm in assembly) and it seems Google hasn't either. While I'm sure it makes sense, I don't think I've ever seen that be enforced. At least in C/C++. Maybe it makes more sense for these stack inspecting garbage collectors but I've also heard of ones that just scan the stack without unwinding anything. I did a test asking Google's AI to generate a complicated C function, put it in godbolt, and there's plenty of push push push push ..... Pop Pop Pop Pop going on
  - rcxdude 1y ago
    Did you compile with optimisations? I think GCC will do a bunch of activity on the stack with -O0, but it'll generally coalesce everything into one push/pop per function with optimisations (not because of any rule, but just because it's faster). alloca and other dynamic stack allocation may break this, but normal variables should in pretty much all just get turned into one block on the stack (with appropriate re-use of space if variable lifetimes don't overlap)

Agingcoder 1y ago

Excellent article as always from the cloudflare blog - engineering without magic infrastructure and ml. One day I will apply ! Compiler bugs are actually quite common ( I used to find several a year in gcc ), but as the author says, some of them only appear when you work at a very large scale, and most people never dive that far.

jgrahamc 1y ago
What's stopping you applying today?
- nevon 1y ago
  Similar to the previous commenter, every time I read a blog post from Cloudflare I end up checking the careers page thinking "this is exactly the kind of work I'd like to be doing". Sadly no openings in my country. I'll keep checking!
  - moomoo11 1y ago
    Pretty sure location is not a factor for these companies. You should apply anyway. I’ve worked with people living in active war zones. If you have the skills, they have the coin. They won’t hire some react guy in X country but someone who can find compiler bugs and save them XX+ million dollars a year? Heck yeah.
    - Degorath 1y ago
      Unfortunately, in 95% cases location IS a factor with bigger companies. I'm in a similar position where I'd like to do something a lot more interesting, but intersection between where the interesting companies have offices and where I'd be willing to live do not really overlap enough justify rooting up my life. (Unless we're talking about "too good to ignore", that's a different story.)
      - moomoo11 1y ago

dreamcompiler 1y ago

Always adjust your stack pointer atomically, kids.

whizzter 1y ago
I guess those that wrote the preemption were on X86 where this doesn't happen thanks to variable length instructions being able to hold the constant and thus relied on the code-gen to do it atomically, then the ARM port had an automatic "split" from a higher level to make things "easy" thus giving us this bug. Nobodys fault really, but bad results ensued.
- Sesse__ 1y ago
  > Nobodys fault really, but bad results ensued. Uh, the fault is entirely in writing an assembler _that is not an assembler_, but rather something that is _almost_ like one but then 1% like an IR instead. It's an unforced error.
  - wbl 1y ago
    Assemblers used to do a ton of stuff back in the day
    - anyfoo 1y ago
      Oh yeah. S/360 assembly almost looks like a high level language sometimes. In MVS, functions of the OS and standard libraries (or its equivalent) were implemented as elaborate macros, with their own invocation syntax, whereas nowadays you'd expect a function that you'd call (dynamically linked or not), with parameters passed in registers. At least in the 90s, there were actually macro assemblers that supported OOP programming in assembly. Borland Turbo Assembler 5.0 comes to mind, if was kind of fun.
      - pjmlp 1y ago
        Those are still around if you go for Assemblers with background in PC culture like NASM, YASM, MASM (still part of MSVC). By the way Embarcaredo still has Turbo Assembler. https://docwiki.embarcadero.com/RADStudio/Athens/en/Turbo_Assembler_Options https://docwiki.embarcadero.com/RADStudio/Athens/en/Turbo_As... Now a thing of the past, but Assemblers for game consoles were also quite powerfull in their macro capabilities. I never liked the UNIX Assembly culture, because naturally as soon as C became a thing, they became the bare minimum required to assemble the generated Assembly out of the C compiler, as another step into the compilation pipeline. All the niceties of macro assemblers came through the other platforms, like being able to use NASM instead of the platform assembler, not even GNU AS nor clang are that great in their abilities as Assemblers beyond the basic stuff.

pengaru 1y ago

For the impatient, here's the fix: https://github.com/golang/go/commit/f7cc61e7d7f77521e073137c6045ba73f66ef902 https://github.com/golang/go/commit/f7cc61e7d7f77521e073137c...

cmckn 1y ago
I noticed this when reviewing the linked issue: https://github.com/golang/go/issues/73259#issuecomment-3100443997 https://github.com/golang/go/issues/73259#issuecomment-31004... Does the Go team have a natural language bot or is this just comment.contains(“backport”) type stuff?
- kbolino 1y ago
  The latter: https://github.com/golang/build/blob/master/cmd/gopherbot/gopherbot.go https://github.com/golang/build/blob/master/cmd/gopherbot/go... (found via https://go.dev/wiki/gopherbot https://go.dev/wiki/gopherbot)
  - 9rx 1y ago
    Although also the former (gabyhelp): https://github.com/golang/oscar/tree/master/internal/gaby https://github.com/golang/oscar/tree/master/internal/gaby
  - etra0 1y ago
    Kinda funny that it requires both "please" and "backport" for it to be considered haha.
chavi2 1y ago
One thing I worry about, probably unnecessarily, is anything with a sense of urgency. HEY GUYS WE JUST FOUND A GOLANG COMPILER BUG AND FATAL PANICS! Everyone is like “Hmm. I need to fix this now.” So, 99% probability it’s what it is. 1% it’s some secret defensive thing because there was a bad stupid zero day someone would get fired over or that could leave the world in shambles if uncovered, or maybe something else needed to be swept under the rug, or maybe someone wants to distract while they introduce a new vulnerability. I don’t think this with CVEs, but when someone’s like “install this patch everybody!” the dim red light flickers on.

riobard 1y ago

What ARM64 machines are you using and what are they used for? Last year you were announcing Gen 12 servers on AMD EPYC (https://blog.cloudflare.com/gen-12-servers/ https://blog.cloudflare.com/gen-12-servers/), but IIRC there weren’t any mentions of ARM64. But now it seems you’re running ARM64 in full production.

zamadatix 1y ago
I'm not Cloudflare, I just read their blog too much. As they hint in the article when mentioning secure boot, they've been deploying Ampere in parallel to AMD for several years now. Purpose wise it seems to be Edge related for efficiency reasons, but maybe they use them for other things too. You can read some more here https://blog.cloudflare.com/designing-edge-servers-with-arm-cpus/ https://blog.cloudflare.com/designing-edge-servers-with-arm-... and here https://blog.cloudflare.com/arms-race-ampere-altra-takes-on-aws-graviton2/ https://blog.cloudflare.com/arms-race-ampere-altra-takes-on-... along with the original evaluation of Qualcomm here https://blog.cloudflare.com/arm-takes-wing/ https://blog.cloudflare.com/arm-takes-wing/
- riobard 1y ago
  Yeah but those are pretty dated. I was under the impression those old Ampere servers are not efficient compared to modern EPYC anymore. So I’m wondering what their current generation of arm64 servers look like :p
EE84M3i 1y ago
I seem to recall Cloudflare hosts their some of their non-edge compute on public clouds? Like control plane stuff. Could be that.

gok 1y ago

The real lesson here should be that doing crazy shit like swizzling the program counter in a signal handler and writing your own assembler is not a good idea.

blinkingled 1y ago
This^. Keith W on Dtrace blog said it a decade ago https://wesolows.dtrace.org/2014/12/29/golang-is-trash/ https://wesolows.dtrace.org/2014/12/29/golang-is-trash/ I like Go but I don't really like their NIH / replace everything with our stuff stance - esp on system tools like assemblers and linkers.
wat10000 1y ago
The general wisdom is that you shouldn't do this stuff yourself, and you should instead rely on tried and tested implementations. But sometimes you're the one who provides the tried and tested implementations. Implementing a compiled language is often one of those times.
themafia 1y ago
Neither of those are "crazy shit." It's just complex because the environment offers specific features like automatic GC with async preemption in a compiled language which pretty much requires it. Complex engineering isn't something to be avoided by default.
- Diggsey 1y ago
  Agree, but I think there is a point to be made here: Go as a language has more subtle runtime invariants that must be upheld compared to other languages, and this has led to a relatively large number of really nasty bugs (eg. there have also been several bugs relating to native function calling due to stack space issues and calling convention differences). By "nasty" I mean ones that are really hard to track down if you don't have the resources that a company like CF does. To me this points to a lack of verification, testing, and most importantly awareness of the invariants that are relied on. If the GC relies on the stack pointer being valid at all times, then the IR needs a way to guarantee that modifications to it are not split into multiple instructions during lowering. It means that there should be explicit testing of each kind of stack layout, and tests that look at the real generated code and step through it instruction by instruction to verify that these invariants are never broken...

renewiltord 1y ago

Great technical blog. Good pathway for narrative, tight examples, description so clear it makes me feel smarter than I am because so easy to follow though the last time I even read assembly seriously was x86 years ago. Also, fulfills the marketing objective because I cannot help but think that this team is a bunch of hotshots who have the skill to do this on demand and the quality discipline to chase down rare issues. I assume these are Ampere Altra? I was considering some of those for web servers to fill out my rack (more space than power) but ended up just going higher on power and using Epyc.

wat10000 1y ago

I would have thought that unwinding would use the frame pointer and this wouldn't be a problem.

mperham 1y ago
The frame pointer was updated non-atomically in two asm ops. An async interruption between the two ops would lead to a corrupt frame pointer.
- wat10000 1y ago
  So it was. The article never mentions the frame pointer and I'm familiar with compilers that load the saved value from the stack in the epilog, rather than adjusting it arithmetically. But they do have an assembly listing showing the two-step arithmetic adjustment for both the stack pointer and frame pointer. But I'm not sure that matters, because the unwind code they show uses the stack pointer rather than the frame pointer anyway.

berz01 1y ago

[flagged]

mperham 1y ago

Did they ever explain why netlink was involved? Or was that a red herring?

Sesse__ 1y ago
The stack in that specific function was big enough to trigger the bug.
drob518 1y ago
Seemed like a red herring. They were able to reproduce it without any libraries. Might have just been net link forcing the stacks to a certain size and that made the bug visible.
syncsynchalt 1y ago
The netlink function uses a larger stack than most. Their repro case required a stack adjustment larger than 1<<12 (4kiB).

brcmthrowaway 1y ago

I don't get it, how were the machine threads being stopped in thr middle of two instructions? This is baremetal, right?

adgjlsfhk1 1y ago
go uses interrupts for GC notifications
purplesyringa 1y ago
Signals.
- ahoka 1y ago
  That's why the old advice was not to use signals and threads together, if you can avoid it.

lordnacho 1y ago

> This was a very fun problem to debug. I'm sure it was a relief to find a thorough solution that addressed the root cause. But it doesn't seem plausible that it was fun while it was unexplained. When I have this kind of bug it eats my whole attention. Something this deep is especially frustrating. Nobody suspects the standard library or the compiler. Devs have been taught from a young age that it's always you, not the tools you were given, and that's generally true. One time, I actually did find a standard library bug. I ended up taking apart absolutely everything on my side, because of course the last hypothesis you test is that the pieces you have from the SDK are broken. So a huge amount of time is spent chasing the wrong lead when it actually is a fundamental problem. On top of this, the thing is a race condition, so you can't even reliably reproduce it. You think it's gone like they did initially, and then it's back. Like cancer.

akerl_ 1y ago
It feels like this comment was almost a purely additive anecdote of your own experience with a similar kind of issue, but you've spoiled it by deciding to tell the author that they're incorrect about how they felt during the process? Maybe different people find different things fun.
- lordnacho 1y ago
  Not saying he's wrong, sometimes the word "fun" connotes something slightly different what what it literally means. "Satisfying" is something I'd use for the end state. Maybe "challenging" for the intermediate state. But while you're in a high-pressure situation that you don't understand, that is rarely "fun" in the literal sense. You wouldn't pay to be given compiler race condition bugs, right?
  - akerl_ 1y ago
    Maybe stop digging here and just let it be fun for the author?
  - a10c 1y ago
    > Not saying he's wrong https://heinen.dev/ https://heinen.dev/ - I’m Thea “Teddy” Heinen (she/her or they/them)!

yalok 1y ago

Classic problem of non-atomic stack pointer modification. Used to have a lot of fun with those 3 decades ago.

quotemstr 1y ago

This problem strikes me more as a debuginfo generation bug than a "compiler" bug. > After this change, stacks larger than 1<<12 will build the offset in a temporary register and then add that to rsp in a single, indivisible opcode. A goroutine can be preempted before or after the stack pointer modification, but never during. This means that the stack pointer is always valid and there is no race condition. Seems silly to pessimize the runtime, even slightly, to account for the partial register construction. DWARF bytecode ought to be powerful enough to express the calculations needed for restoring the true stack pointer if we're between immediate adjustments.

sauercrowd 1y ago
> This problem strikes me more as a debuginfo generation bug than a "compiler" bug. But isn't that the same thing here? The bug occurred in their production workflows, not in some specific debug builds, so with that seems pretty reasonable to call it a compiler bug?
- quotemstr 1y ago
  Thanks. I think of unwinder information as debuginfo even though, as you point out, it's used outside of debugging contexts all the time. :-) As for the actual bug: Unless you're unwinding the stack by walking the linked list of frames threaded through the frame pointer, then each time you unwind a level of the stack, you need to consult a table keyed on instruction pointer to look up how to compute the register contents of the previous frame based on register content of the current frame. One of the registers you can compute this way is the previous frame's stack pointer. I haven't looked in depth at what the Go runtime is doing exactly, but at a glance, I don't see mention of frame pointers in the linked article, so I'm guessing Go uses the SP-and-unwind-table approach? If so, the real bug here is that the table didn't have separate entries for the two ADDs and so gave incorrect reconstruction instructions for one of them. If, however, frame pointers are a load-bearing part of the Go runtime, and that runtime failed to update frame pointer (not just the stack pointer) in the contractually mandatory manner, well, that's a codegen bug and needs a codegen fix. I guess I just don't like, as a matter of philosophy if not practical engineering, having frame pointers at all. Without the frame pointer, the program already contains all the information you need to unwind, at no runtime cost --- you pay for table lookups only when you unwind, not all the time, on straight-line code. The purist in me doesn't like burning a register for debugging, but you have to use the right tool for the job I guess.

pfdietz 1y ago

I see something like this and I wonder "what testing methodology would have found this?" It has to be general, not something that would involve knowing what the bug was ahead of time.

syncsynchalt 1y ago
When your scale is large enough, you move to "what monitoring methodology will find this?" When you're doing enough transactions you start to see a noise floor of e.g. bit flips from cosmic rays, and looking for issues involves correlating/categorizing possible software failures and distinguishing them from the misbehavior of hardware.
- pfdietz 1y ago
  I meant, what testing methodology could the compiler writers have used, so it was caught before it went to users. The feedback loop here should be: novel bug comes in ==> determine how existing testing was deficient ==> modify the testing in a general way that would have found this bug ==> run these modified tests in the background to see if anything similar was missed. Bugs should be used as indicators that regions (as large as possible) of bug space have been inadequately covered.

MarkSweep 1y ago

I wonder if Go had a mode where you make it single step every instruction and trigger a GC interrupt on every opcode. That would make it easier to find these kinds of bugs.

defleopold 1y ago
[flagged]

neuroelectron 1y ago

I've seen only one race condition in my career and it always surprises me how it is even found.

Vipsy 1y ago

One thing that often gets missed is how hard it is to even suspect the compiler as the root cause. Most engineers waste hours chasing bugs in their own code because we’re trained to trust our tools. This mindset alone can make these rare compiler bugs much trickier to find.

kmarc 1y ago
There are certain professions where the compilation process is (ab)used to optimize to a point where these bugs seemingly surface more often. In the HFT sphere i haven't talked to a company that hasn't reported (bragged about finding) a super weird gcc/clang bug. Well, also, at my last job we used a snapshot version of the compiler, bc... Any nanoseconds matters.
- hshdhdhehd 1y ago
  In HFT might you keep the bug fix secret so other HFTs cant benefit from it.
  - kmarc 1y ago
    I saw both. One of the top firms wanted that, another I worked at we did report (of course with a scratched minimal reproducible example) The thing is, it's quite unlikely that your competitor hits the exact same bug. The cost of us having to keep upstream patched, tested isn't justified. Also in HFT world there are some very similar patterns across competing companies, yet, we just saw TernFS coming out from XTX, with not much fear of competitors benefiting from it more than they do.
pjmlp 1y ago
In the early PC days we suspected them a lot given how manually writting Assembly was still much better, in many cases. I found out a bug on Turbo Pascal 6, where if you declare a variable with the same name as the function name, then the result was random garbage. For those that don't know Pascal, the function name has to be assigned for the result value, so if a local variable with the same name is possible, then you cannot set the return value. Something like this https://godbolt.org/z/s6srhTW66 https://godbolt.org/z/s6srhTW66 (* In Turbo Pascal 6 this would compile *) function Square(num: Integer): Integer; var Square: Integer; begin Square := num * num; (* Here the local variable gets used instead *) end;

Bengalilol 1y ago

I always appreciate articles like this, where you can clearly see the engineer’s way of thinking. I was just puzzled by the middle part of the article, where they start investigating their code but seem to overlook the fact that it only happens on ARM64. Still, I understand that it’s professional to proceed step by step logically. Great article, it was a pleasure reading it!

mixedbit 1y ago
Hard to reproduce bugs often depend on an order of events or timing. Different architecture can trigger different order of execution, but this doesn't mean the bug is not in the application.

me2too 1y ago

Great write-up

anthk 1y ago

I miss the Delve debugger for OpenBSD 386 BTW.

bradley13 1y ago

I find it interesting, how rare it has become to find s compiler bug. For me, at least, it used to be a regular event. Even Java, as widespread as it is, I have made half-a-dozen reports. None in the last several years, though. Better testing? The sheer scale of software being produced?

lou1306 1y ago
Linus's law [1]? When it comes to compilers for mainstream languages, the userbases are so large that they will explore a surprisingly large portion of the compiler's state space. But definitely, better engineering and QA practices must also help here. [1] https://en.wikipedia.org/wiki/Linus%27s_law https://en.wikipedia.org/wiki/Linus%27s_law

wy1981 1y ago

Great find and writeup. As an aside, this is the type of a problem that I think model checkers can't help with. You can write perfect and complicated TLA+/Lean/FizzBee models and even if somehow these models can generate code for you from your correct models you can still run into bugs like these due to platform/compiler/language issues. But, thankfully, such bugs are rare.

jraph 1y ago
Yep. Model checking is for checking that your design is sound, basically, not at all the implementation. For the implementation, you can use certified compilers like CompCert [1], but: - you still have to show your code is correct - there are still parts of CompCert that are not certified [1] https://compcert.org/ https://compcert.org/

maguro_01 1y ago

An x86-64 Windows 11 machine trying to access a previously available Website now always produces a Cloudflare "obsolete protocol" error on the ordinary attempt. Al browsers get the same error. Did your fix break something?

maguro_01 1y ago
Sorry, all browsers.