search

dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.09k stars 1.56k forks source link

Clean up IA32 Assembler's `code_` #48852

Open ghost opened 2 years ago

ghost commented 2 years ago

assembler_ia32.h has a field, code_, which is not present in any of the other assemblers:

https://github.com/dart-lang/sdk/blob/618db8a41d68fd37ff5bb228c69486721be3a636/runtime/vm/compiler/assembler/assembler_ia32.h#L1093

It is only read in one place: https://github.com/dart-lang/sdk/blob/618db8a41d68fd37ff5bb228c69486721be3a636/runtime/vm/compiler/assembler/assembler_ia32.cc#L2699-L2704

And set in one place: https://github.com/dart-lang/sdk/blob/618db8a41d68fd37ff5bb228c69486721be3a636/runtime/vm/object.cc#L17254-L17256

Note how we're setting the field through a getter: https://github.com/dart-lang/sdk/blob/618db8a41d68fd37ff5bb228c69486721be3a636/runtime/vm/compiler/assembler/assembler_ia32.h#L1046

This is already fishy, but this quirk is also causing a different bug, whereby we incorrectly set the Code object in Dart frames to the current Code, instead of the Code being called.

For example, in FfiCallInstr we commonly do the following across all architectures: https://github.com/dart-lang/sdk/blob/618db8a41d68fd37ff5bb228c69486721be3a636/runtime/vm/compiler/backend/il_ia32.cc#L1039-L1041

Except, on IA32 EnterDartFrame calls the above PushCodeObject which we saw always pushes whatever is in code_, and not what is in CODE_REG, as the comment otherwise claims (which is correct on other archs.): https://github.com/dart-lang/sdk/blob/618db8a41d68fd37ff5bb228c69486721be3a636/runtime/vm/compiler/assembler/assembler_ia32.cc#L2706-L2709

This ultimately has the effect that FFI ExitFrames have a duplicate FfiTrampoline Code object in them where they should have Null, which in turn affects stack traces, etc.

Aside: This might also affect other users of EnterDartFrame, but I haven't checked.

ghost commented 2 years ago

This also affects all users of EnterStubFrame: https://github.com/dart-lang/sdk/blob/618db8a41d68fd37ff5bb228c69486721be3a636/runtime/vm/compiler/assembler/assembler_ia32.cc#L2732-L2734

Such as: https://github.com/dart-lang/sdk/blob/618db8a41d68fd37ff5bb228c69486721be3a636/runtime/vm/compiler/stub_code_compiler_ia32.cc#L70-L72 Which clearly doesn't do the same as it does in the other architectures.

mkustermann commented 2 years ago

... we incorrectly set the Code object in Dart frames to the current Code ...

There may be a misunderstanding here. The code object in a frame is representing the code object of that frame's function (not the caller's code object).

On most of our architectures (x64, arm, arm64) the caller is populating this code object as part of the call sequence: In order to call a function, the caller has to load the Functions Code and from there the Codes entrypoint address. The caller therefore has to load the code object anyway, so it loads it into the specified CODE_REG register - which makes it available to the callee (i.e. the callee can simply push CODE_REG - which is its own code object).

Now on ia32 we do not use code objects as such. Callers (e.g. a static call) will invoke its target directly - no need to get the entrypoint from a code object. The callee therefore has to get it's own code object in a different way, namely in the way you described above.

This is by-design.