← Back to context

Comment by CBLT

3 days ago

> the CPUID example in the article [...] forgot to bind ECX as an input.

I'm not really familiar with this stuff, but the example uses what it calls a "pin" (which in their docs is a type of "binding") on ECX before calling CPUID.

You don't really need to be familiar with either "this stuff" or Odin to spot that this clearly takes a single parameter named "leaf" and that's the input, which goes in EAX. However CPUID may care about ECX as input and that's only used as an output in this uh, "template".

Here's Rust implementing this same feature:

https://doc.rust-lang.org/src/core/stdarch/crates/core_arch/...

Rust provides this for both x86-64 and the original 32-bit x86 and this is a function, not an Odin-style "template" but hopefully this helps show what you're supposed to do.

[Edited to add the Rust example]

  • I'm not sure the example template was supposed to be canonical, vs demonstrating multiple output destructuring. Certainly the actual instruction definition in the checker library seems to understand that there could be two possible inputs:

    https://github.com/odin-lang/Odin/blob/4247507dd5e31c9fd8716...

    But I'm also not entirely sure why the example should not have compiled. It seems to me that the idea here is to be able to define a typed set of something equivalent to a function that inlines some assembly, but nothing about that inherently requires that the number of input or output parameters to the template match the parameters in the underlying assembly calls. There's no reason (in my mind anyway) why this shouldn't be a perfectly valid template:

        // Returns the extended feature flags obtained by calling CPUID
        // with EAX=7 and ECX=1
        cpu_extended_feature_flags :: asm() -> (a, b, c, d: u32) [
            a = %eax,
            b = %ebx,
            c = %ecx,
            d = %edx,
        ] {
            mov %eax 0x7
            mov %ecx 0x1
            cpuid
        }

    • > But I'm also not entirely sure why the example should not have compiled

      The source you presented seems fine - it's explicitly setting the register. The trouble with the cpuid definition in the article is that it just doesn't set ECX at all

      It does actually seem as though Odin is intended to notice this problem but maybe is fooled that the register is pinned (because we want its result value) and so the diagnostic doesn't trigger.

      Reading this code reminded me that my annual summer leave ends this weekend because it would get so much review feedback if he worked with me. "Commenting out" blocks of code is NOT OK and neither are "if (false)" blocks.

      9 replies →