Return record #3155
Return record #3155
Conversation
See modelica#3130 (comment) Note that this should be fairly straightforward to implement.
| @@ -2229,8 +2229,8 @@ \subsubsection{Records}\label{records} | |||
| Arrays cannot be mapped. | |||
| \end{itemize} | |||
|
|
|||
| Records are passed by reference (i.e.\ a pointer to the record is being | |||
| passed). | |||
| Records that are the return value of the C function are returned by value. | |||
There was a problem hiding this comment.
I don't think this should be allowed as I don't see that we should rely on any definition of CRec in the signature. See #3154.
There was a problem hiding this comment.
I don't see a problem with returning structs by value, as it has been done in C since at least the 1980s.
There was a problem hiding this comment.
This only works well if nested records are also stored as struct und not as pointer in the surrounding struct. Else the question about the memory management arises again.
There was a problem hiding this comment.
This is not a concern about returning records by value in C in general, but a concern about using this mechanism in Modelica's external function protocol. The problem we have is that the external function should come with a prototype, and that prototype will have a definition of the record type that the tool generating the call to the external function cannot reasonably be expected to be aware of. (At least I don't want a solution where the tool needs to scan files included via the Include annotations in search of a the definition of the record used as the external function's return type.)
This test program demonstrates the problem:
struct A
{
double x;
int y;
};
struct B
{
double x;
int y;
};
struct A
f()
{
struct A a;
a.x = 0.5;
a.y = 1;
return a;
}
int
main()
{
struct B b = f();
return 0;
}
Clang rejects this with:
return_record.c:25:12: error: initializing 'struct B' with an expression of incompatible type 'struct A'
struct B b = f();
^ ~~~
Beside the problem that the program is rejected by Clang, we have the potential inefficiency of returning big structs by value.
Just for clarity, this is the way I believe it needs to be done:
void
f(void* a)
{
struct A* a_ = (struct A*)a;
a_->x = 0.5;
a_->y = 1;
}
int
main()
{
struct B b;
f((void*)(&b));
return 0;
}
There was a problem hiding this comment.
That is horribly clunky.
First and foremost using void*-pointers is just asking for trouble - and #3198 gives a way of ensuring that the records are consistently defined.
For the possibility of return structs - what many/most/all(?) C-compilers internally do when you declare a function with a large struct as return value is to send in the return address to the function.
So, basically the C-compiler already do the same as this proposal, but automatically, type-safe, and only when needed. Having that as an option for users makes a lot more sense.
|
I think this issue has received too little attention, considering how many MA members that I think would have valuable input to the discussion. Could someone please help me ping the right people? For example, maybe @beutlich, @sjoelund, @christoff-buerger or @andreas-junghanns can relate to the question of how one should return records from the implementation of an |
|
I have problems with your example involving records My suggestion is that we look if we could make a name mapping, telling the Modelica translator that "for this record please use this type name in the generated code". That allows you to define the type in C and your generated code will adapt in a clean way. By the way, returning |
There are two cases for multiple outputs:
|
Yes, this is the point of the example.
This is an alternative I have also considered, but I thought it would be considered too complicated compared to using a
Yes, there is no question about returning struct by value being perfectly fine in C in general. The question is specific to returning records from Note that we must anyway support returning records via pointer to allocated storage, in case there are multiple outputs from the function. Thus there is hardly any added value in also allowing the records to be returned by value. |
Fine, all in areement here.
I mst admit I have never quite understood the name-mangling of record members that some tools seem to apply (
But here the question is also how we most easily adapt to what we think is the common programming style in C. If returning structs is common, then that is the natural option. If returning by in/out pointer arguments is common, then we should do that. Maybe we need to search for common real-world patterns. |
|
There is a reason why the FMI group avoids structs. Maybe they have some input? Here we are even talking about language border crossing structs (Modelica records <-> C structs). My assumption is: It needs guided/configured code generation and compilation on the fly on both ends (as we know from techniques like CORBA etc). |
Note that in Modelica 3.4 we added a way to avoid structs - even if the Modelica function has records - by sending each record member separately. So in one way one can argue that structs are only used when the user wants to. Having a way to specify the C-name of the record, including the header containing its definition (and either with standardized names for the elements or a way of specifying them) would fix the remaining issue. However, that is somewhat orthogonal to this PR about returning records from external funcitons.
Guided code generation is needed for some cases, but given that the main use case is that you have a specific existing C-library that you want to call I don't think it is needed in most cases for Modelica. It is clearly interesting if we want to add more generic support of calling other languages, e.g. JNI for Java; https://modelica.org/events/modelica2006/Proceedings/sessions/Session5a3.pdf |
|
In another attempt to reach out for input from MAP-FMI people with the deep understanding of this kind of topic, perhaps @pmai has some thoughts to share? (Previously asked @andreas-junghanns, but so far no response.) For background in a nutshell: the topic is how to handle Modelica records (possibly nested) in the external C interface. |
I don't understand what you are asking for, as the deep understanding has already been given. Returning records has been part of C since at least the 1980s, and is documented in ABIs. The goal of this PR isn't to mandate that function return records, but to allow it - just being a few decades after C. Thus it does not matter that many coding styles don't use it in C (since the return value is a status code as in FMI, and structs are often input/output arguments). In C++ they are used more extensively (also for classes) requiring special optimizations like RVO and move-semantics. |
I am hoping that others would also see that there is a clear problem with passing by value when the code we generate for calling the external function doesn't have access to the same struct definitions used in the prototype of the external function (provided by means of a header given in the Note that we already use |
That is not what this PR is about; it's about returning records as stated in the name. Specifically this PR is about ensuring that external function can handle return values using the same record definition that are already allowed for arguments. That is perfectly supported by the C-standard and has been for decades. Having an Include-annotation would help both in terms of C-standard conformance, but in practice it works for most platforms even without that. |
I'm sorry, but I can't be in favor of the change as long as I don't see how the two could be unrelated. Returning a record means that there would be a struct in the return type of the external function's prototype, and it just doesn't seem feasible to be to generate clean code for calling such a function without having full access to the definition of that struct. It is because I don't see the feasibility of doing that (way too many annotations needed to to make it work), that I think returning records by value from external Modelica functions (not in C in general, of course) is not a good idea. By only returning records by pointers to allocated output memory one would get a realistic setting for both implementors of external functions and for tools that need to generate the code for calling them. |
Sending in a pointer to the struct, as currently allowed, has the exact same issues. I'm not against adding Include-annotations for records, but it is a separate issue.
Not true at all. That has the same problems (with alignment, layout, and strict C conformance for names etc; as we already have for pointers to structs and is proposed for returning records), and adds the possibility of incompatible memory allocation on top of that. (And in practice also requires manual memory management, that people often mess up.) It's a really really bad idea. |
Yes, the issues are the same, but passing a
No, as allocated memory is passed to the external function, no manual memory management is required on the external function side. I don't see any difference to the way we return arrays. |
That would be a step back. Instead I want to take two separate steps forward:
Switching to
Side-note: External objects are different as it's only the external code that accesses the contents of the external object, but these structs are sent between external code and Modelica code, and modified by both. Proposing In contrasts extending the declarations of structs in general, #3198 , is a straightforward extension, that takes less time to implement than concluding this pseudo-discussion.
If "returning records by pointers to allocated output memory" means only handling records by pointer to memory allocated by the caller, it only means that we still have the same minor problem as today - whereas returning records wouldn't introduce any new ones. Note that this is still only relevant if the external call wants to return a struct it's an entirely optional proposal for modelers. |
|
As seen in #3154, we don't really have any working mechanism at all for records at the moment, so whatever we do here should be seen as fixing that problem, not extending upon something that is already working. I think the best way to proceed is to first settle #3198, and then return to this issue. (I expect that settling #3198 will end up in a clear decision between going with void pointers or going for the suggested complete specification of C structs.) |
|
We can handle #3198 first, but is not blocking this issue. The reason is that they are largely independent, and thus there is little or no overlap, and therefore no or few merge conflicts.
In practice we have a working mechanism for records at the moment and have had since Modelica 1 - and they are used. However, that mechanism relies on specifics of ABIs on most used platforms, but is not fully according to the C-standard. That can be improved in a backwards compatible way in #3198 but it is not needed |
| @@ -2229,8 +2229,8 @@ \subsubsection{Records}\label{records} | |||
| Arrays cannot be mapped. | |||
| \end{itemize} | |||
|
|
|||
| Records are passed by reference (i.e.\ a pointer to the record is being | |||
| passed). | |||
| Records that are the return value of the C function are returned by value. | |||
|
To clarify how ABIs impact this. However, the main idea is to not care and let C-compilers handle the C-code without trying to be clever. Whether the variable is a struct or not isn't that significant nowadays (it seems that it matters on VAX) - but it somewhat depends on how homogenous the struct is, and as long as the records are "compatible POD" the exact declarations aren't that critical - but #3198 should clear up that. But after some investigating - for return values on x86-64:
Note that the Windows normal ABI will also handle input structs containing two or more doubles by sending a pointer to the struct - so if we say that since some ABIs cannot return a complex number in registers we shouldn't try to declare a C-function returning such a struct, we should logically then also do the same for any complex input. The idea is that by just declaring it as a C-function returning a struct (and fixing the types of records as in #3198 ), we let the C-compiler handle it; if users have such functions. If the C-function takes a pointers to the outputs you don't have to say that it returns something, and can use a different external-declaration - as before. |
Enhancement to allow functions to return record values.
See #3130 (comment)
Note that this should be fairly straightforward to implement.