Closed m8pple closed 2 years ago
mvousden
commented
3 years ago Perhaps I'm misunderstanding, but could one just define a type in the Graphs/GraphType/SharedCode element, and refer to the type from here? You could then define an instance of that type in your relevant CDATA section?
heliosfa
commented
3 years ago Perhaps I'm misunderstanding, but could one just define a type in the Graphs/GraphType/SharedCode element, and refer to the type from here? You could then define an instance of that type in your relevant CDATA section?
You used to have to do this in our "v2" Softswitch for the local device shared code and it was a pain.
When we (you @DrongoTheDog and I) were discussing struct scoping, we decided that it made sense to make the device-level structs available in the device's shared code but that making them available at the global level could cause issues. Unfortunately I can't remember what the issues we identified were, but compile time springs to mind.
Also, within the global SharedCode it might refer to any of the state or properties of devices.
Is this just for sizing? or are there other "legitimate" reasons you can see for devices (and by extension supervisors) to have access to the struct declarations for all device types?
I guess that a potential fix is to emit a general types header (rather than just a message types header) that includes all of the extra structs. However, where does this stop? Does it include all of the edge props/state structs as well?
m8pple
commented
3 years ago Yes, you could create a custom type in Graphs/GraphType/SharedCode, but that type would be completely
independent of the types used for things like messages, states, and properties.
You could replicate the structure of a message type, and then make sure you kept the two definitions in sync, but that is a recipe for problems, as experience suggests that the widths of types get changes slightly or someone inserts another variable, but then forget to update them.
There is also a worry that an implementation might decide to lay out the types
differently in memory - for example, some part compiled in gcc-x86 and some part
compiled in risc-v, which relies on implementation-defined padding and packing
decisions. That's why it is required that all structs are emitted with #pragma pack(1),
as otherwise it is impossible to move the around portably.
(I notice that the orchestrator doesn't emit pragma pack 1. I guess this is less of an issue while externals are not supported, but does suggest some quite interesting edge cases between the Mothership and the devices...)
Also, if you take the approach of having two distinct types you need to either memcpy between the two (wasting time), or type-pun them via pointers, which means the handlers become non-synthesisable in HLS. (A HLS POETS implementation existed in the past, and may exist again in the future).
So the well-known type-name is there to allow handlers to refer to the types
as emitted by the implementation, as it avoids a lot of those problems. Note
that there is also the cTypeName override, though I haven't looked into
whether the orchestrator supports that.
You could then define an instance of that type in your relevant CDATA section?
I assume by CDATA section you mean something like a MessageType or State element? That approach will work with an implementation that does not introspect types and views them as strings (such as the orchestrator), but many other implementations parse and introspect all types and values in order to discover errors, and to enable direct binary generation (rather than getting gcc to do it).
In the v3 XML there was more support for custom type instances through typedefs, but that was seen as too complicated to implement, so was removed in v4. So in v4 the only things allows in the typedDataSpec and typedDataValue fields must be standalone, with the only allowable types coming from cstdint:
m8pple
commented
3 years ago When we (you @DrongoTheDog and I) were discussing struct scoping, we decided that it made sense to make the device-level structs available in the device's shared code but that making them available at the global level could cause issues. Unfortunately I can't remember what the issues we identified were, but compile time springs to mind.
It's done in various other implementations (at least two simulators in graph_schema, poets_eco_system, and the reference parser/implementation developed during the PIP20 discussions.
When exposed as global type names, you get {graphTypeId}_{deviceTypeId}_properties_t, so it shouldn't be
possible for them to conflict. Within each handler there are supposed to be aliases DEVICE_STATE_T and
DEVICE_PROPERTIES_T, but those are local to each handler (it appears unspecified whether they are in
scope for the SharedCode section of a device - I think they were intended to be, but it is a little
implicit).
Is this just for sizing? or are there other "legitimate" reasons you can see for devices (and by extension supervisors) to have access to the struct declarations for all device types?
It's mainly todo with sizing, and for things like static assertions. It's useful to check at compile-time that the parameters associated with one handler are still compatible with the current configuration of another parameter.
It was more useful with the V3 support for typedefs, but a few of those use-cases have disappeared with the simplification for V4.
In cases where you are working around the lack of expressiveness, it is still useful to be able to static_assert that your type-punning is actually valid. For example, if a particular struct is being serialised in a raw byte message (to get around lack of unions and typedefs), it is useful to check that the size of the struct being sent is the same as the expected one being received.
I guess that a potential fix is to emit a general types header (rather than just a message types header) that includes all of the extra structs. However, where does this stop? Does it include all of the edge props/state structs as well?
Yes, the spec expects named types for edge properties and state too:
This follows on from #231.
This was done on development_dt10_branch (fd0196afba08e70beafd4b87090ee15c0befb00f), as otherwise I cant get through parsing through to compilation. The branch has the current 1.0.0-alpha merged in.
Some existing graphs refer to various global types in order to work out things like the maximum size of things at compile-time. Also, within the global SharedCode it might refer to any of the state or properties of devices.
At the moment it looks like only the types for the device currently being compiled are emitted, which breaks applications that use multiple device types and refer to those types from across handlers.
For example, this is used in an APSP implementation to statically work out batch size:
https://github.com/POETSII/poets_improvement_proposals/blob/2a355282f3aa3912bdacf4ec50bf25199bdf310d/proposed/PIP-0020/xml/ic/apps/apsp_vec_barrier_150_10.xml#L12-L15
Current behaviour:
Microlog: