[XL] Change in dart/sdk[main]: [vm/service]: Add _getFfiStructLayout RPC for FFI struct field layout
Daco Harkes (Gerrit)
Daco Harkes added 5 comments![]( "Open in Gerrit")
@rma...@google.com Since you've made the most changes to `NativeType` in the last years. What do you think of the architectural options here?
Background: We need some display information to show to users for structs and their fields. And the VM service relies on the `NativeType` hierarchy calculations to figure out the field offsets and sizes. Which of the options listed in the comments below do you prefer? Or do you have another idea?
.map((f) => f.field?.name.text ?? f.getter!.name.text)Maybe add the fallback on `class CompoundField` instead. `String get name =>`
.toList(),Nit you can use `[for (final f` instead of `map` and `toList`.
static const NativeType* CompoundFromPragma(Let me think about the approach here architecturally:
Before this change, the NativeType _only_ has information needed for computing the calling convention. And this method only produces a NativeType.
With this change this method has two side effects, member names and user-readable dart types.
This feels kind of weird.
Some ideas on how to make the architecture better.
Option 1:
- Add the field names to `NativeCompoundType` changing `const NativeTypes& members_;` to a new class that holds both a native type and a name per member entry.
- And _only_ return the `NativeType`.
- And make the VM service interact with the `NativeType`.
- And have a mapping function from `NativeType` to some display name in the VM service.
- We might then also need to add a name `NativeCompoundType` itself, so that you can display the name of structs and unions.
- And then we should add such info to the `PrintTo` methods as well.
=> This ensures that we have a proper abstraction. The VM service only interacts with `NativeType` and it contains all the needed info also for debugging.
=> It also slightly slows down the compiler, because we're always parsing those names. (We're doing that already by embedding the names in the ffi struct fields pragma anyways. So it would be consistent.)
=> We might be able to use those names in the existing code to give more useful error messages.
Option 2: Scope the debugger-useful names only to the VM service.
- Write a `DebuggerCompoundFromPragma` that calls `CompoundFromPragma` and then also parses the pragma again and constructs a new data structure that copies out all the relevant data into a new consistent data structure.
Pros:
- Separation of concerns.
Cons:
- Parsing the pragma twice. And some code duplication for that. Maybe some of the code duplication can be avoided.
Option 3:
Introduce a `DebuggableNativeStructType : public NativeStructType` that holds on to the names and return that from `CompoundFromPragma` if `bool debug_info=true` as a param.
Cons:
- Complicates the type hierarchy.
I think I'm leaning towards option 1.
WDYT @bkony and @rmacnak?
Related details
- Ben Konyi
- Nourhan Hasan
- Ryan Macnak
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Macnak (Gerrit)
Ryan Macnak added 1 comment![]( "Open in Gerrit")
@rma...@google.com Since you've made the most changes to `NativeType` in the last years. What do you think of the architectural options here?
Background: We need some display information to show to users for structs and their fields. And the VM service relies on the `NativeType` hierarchy calculations to figure out the field offsets and sizes. Which of the options listed in the comments below do you prefer? Or do you have another idea?
Maybe I'm not understanding the usage, but I would have expected changes to the output of getObject, not a new RPC, to be able to show the state of FFI objects.
(Compare "Records and the VM Service".)
Related details
- Ben Konyi
- Nourhan Hasan
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |