Clarify nested records.

[HansOlsson]

Closes #3128
As explained this is variant A and consistent with the original design in Modelica 1.1-1.4, and also implemented in OpenModelica and Dymola.

[henrikt-ma]

With this clarification, there seems to be a substantial risk of alignment problems. Isn't this something we should at least comment as a potential problem?

[HansOlsson]

With this clarification, there seems to be a substantial risk of alignment problems. Isn't this something we should at least comment as a potential problem?

Mixing Integer and Real already has that problem, and I couldn't see any specific comments related to that in actual ABIs
https://itanium-cxx-abi.github.io/cxx-abi/abi.html#pod

I would even say that having a pointer at that place would be more problematic, since there are both 32-bit and 64-bit architectures and they have different pointer sizes with different alignment.

[henrikt-ma]

I would even say that having a pointer at that place would be more problematic, since there are both 32-bit and 64-bit architectures and they have different pointer sizes with different alignment.

But isn't that what the different platform directories below LibraryDirectory are supposed to distinguish?

[HansOlsson]

I would even say that having a pointer at that place would be more problematic, since there are both 32-bit and 64-bit architectures and they have different pointer sizes with different alignment.

But isn't that what the different platform directories below {{LibraryDirectory}} are supposed to distinguish?

Obviously you will link with different binaries for 32-bit and 64-bit (possibly after finding the right version in a fat binary).

But the point was that having a pointer wouldn't solve anything - but instead introduce new alignment issues for them (including possible padding).

[henrikt-ma]

I don't have enough experience with C ABI compatibility issues to either approve or request any specific changes on this. Hoping that there are others who can provide more insightful comments.

Of course, if there's a phone meeting decision in favor of this, I can provide an approving review reflecting that.

[henrikt-ma]

While we're looking at these formulations, maybe we could also make this a complete sentence:

A Modelica record class that contains simple types, other record elements, is mapped as follows

I suppose the first comma can simply be replaced by or, right?

[henrikt-ma]

Another related issue is that the Return Type Mapping table only has a reference to section 12.9.1.3 for records in C. That section doesn't really cover the case of returning records, but at least the reference suggests that records can be returned. If 12.9.1.3 should be interpreted to say that a record is also returned by reference (that is, a pointer), how is memory managed in that case?

Regardless whether a record is returned by reference or value, I think the table entry should be replaced by a more direct statement, and then a reference to 12.9.1.3 can be mentioned after the table for details on how the C record type is constructed based on the Modelica type. Just like we use T to represent a generic array element type in the table, I think we should use R to represent a generic record.

Finally, I suggest that we replace the following sentence in 12.9.1.3 by a one row table with distinction between input and output formal parameters, similar to how other type mappings have been described above:

Records are passed by reference (i.e. a pointer to the record is being passed).

[HansOlsson]

Another related issue is that the Return Type Mapping table only has a reference to section 12.9.1.3 for records in C. That section doesn't really cover the case of returning records, but at least the reference suggests that records can be returned. If 12.9.1.3 should be interpreted to say that a record is also returned by reference (that is, a pointer), how is memory managed in that case?

That's two separate issues:
The simple solution for Modelica would be to just return records as values (unless it's specified that it's in a different position), and I believe that we could add that. C supports returning structs, and it is very common in C++, so ABIs and compilers should support it.

The second issue is then how is memory managed in C in that case, and the answer is that it depends. Small structs can be returned in registers, but larger ones are allocated by the caller and returned by reference - but that is done by the C-compiler and we shouldn't have to care about in Modelica. (In C++ there are additional considerations for non-trivial classes for calls.)
What is considered 'small' depends on the C-compiler (and the ABI it supports), and the important point is that this is a decision that should be left to C-compilers.

Note that as far as I can see already 2nd edition K&R supported that (I don't have 1st edition); so it has been supported for decades.

Regardless whether a record is returned by reference or value, I think the table entry should be replaced by a more direct statement, and then a reference to 12.9.1.3 can be mentioned after the table for details on how the C record type is constructed based on the Modelica type. Just like we use T to represent a generic array element type in the table, I think we should use R to represent a generic record.

Agreed.

[henrikt-ma]

Regardless whether a record is returned by reference or value, I think the table entry should be replaced by a more direct statement, and then a reference to 12.9.1.3 can be mentioned after the table for details on how the C record type is constructed based on the Modelica type. Just like we use T to represent a generic array element type in the table, I think we should use R to represent a generic record.

Agreed.

With some scope creep, it would be a further improvement to specify that the type mapping goes specifically to void*. Otherwise, I see no way of avoiding warnings (and potential errors) about incompatible pointer types. If this is too much scope creep, I'll open a separate issue.

[HansOlsson]

A Modelica record class that contains simple types, other record elements, is mapped as follows

I suppose the first comma can simply be replaced by or

I first thought about "and" or possibly "and/or" before realizing that it should just be:
"A Modelica record class is mapped as follows"

Additionally it should talk about its components, not elements - in case someone declares a class nested in the record.

[HansOlsson]

With some scope creep, it would be a further improvement to specify that the type mapping goes specifically to void*. Otherwise, I see no way of avoiding warnings (and potential errors) about incompatible pointer types. If this is too much scope creep, I'll open a separate issue.

The use of "void*" shall be kept to a minimum.

I don't see that there is an issue for different structs here: The C standard defines that identical structs are the same, I verified that the C++ standard also has this for "layout-compatible" structs (same contents) and allow treating pointers to them interchangeably.

[henrikt-ma]

With some scope creep, it would be a further improvement to specify that the type mapping goes specifically to void*. Otherwise, I see no way of avoiding warnings (and potential errors) about incompatible pointer types. If this is too much scope creep, I'll open a separate issue.

The use of "void*" shall be kept to a minimum.

I don't see that there is an issue for different structs here: The C standard defines that identical structs are the same, I verified that the C++ standard also has this for "layout-compatible" structs (same contents) and allow treating pointers to them interchangeably.

At least Clang rejects this test program:

struct A
{
  double x;
  int y;
};

struct B
{
  double x;
  int y;
};

double
f(const A* r)
{
  return r->x * r->y;
}

int
main()
{
  B b;
  b.x = 2.0;
  b.y = 5;
  double c = f(&b);
  return 0;
}

This is what it says:

➜  struct-foo clang++ foo.cpp
foo.cpp:25:14: error: no matching function for call to 'f'
  double c = f(&b);
             ^
foo.cpp:14:1: note: candidate function not viable: no known conversion from 'B *' to 'const A *' for 1st argument
f(const A* a)
^
1 error generated.

If I convert the program to C, I get the following warning from Clang:

➜  struct-foo clang foo.c
foo.c:25:16: warning: incompatible pointer types passing 'struct B *' to parameter of type 'const struct A *' [-Wincompatible-pointer-types]
  double c = f(&b);
               ^~
foo.c:14:19: note: passing argument to parameter 'r' here
f(const struct A* r)
                  ^
1 warning generated.

This is why I thought making use of the dreaded void* would actually be the right thing to do in this case.

[HansOlsson]

With some scope creep, it would be a further improvement to specify that the type mapping goes specifically to void*. Otherwise, I see no way of avoiding warnings (and potential errors) about incompatible pointer types. If this is too much scope creep, I'll open a separate issue.

The use of "void*" shall be kept to a minimum.
I don't see that there is an issue for different structs here: The C standard defines that identical structs are the same, I verified that the C++ standard also has this for "layout-compatible" structs (same contents) and allow treating pointers to them interchangeably.

At least Clang rejects this test program:

struct A
{
  double x;
  int y;
};

struct B
{
  double x;
  int y;
};

double
f(const A* r)
{
  return r->x * r->y;
}

int
main()
{
  B b;
  b.x = 2.0;
  b.y = 5;
  double c = f(&b);
  return 0;
}

This is what it says:

➜  struct-foo clang++ foo.cpp
foo.cpp:25:14: error: no matching function for call to 'f'
  double c = f(&b);
             ^
foo.cpp:14:1: note: candidate function not viable: no known conversion from 'B *' to 'const A *' for 1st argument
f(const A* a)
^
1 error generated.

If I convert the program to C, I get the following warning from Clang:

It's more complicated - in C++ the structs are layout compatible (not compatible); so the compiler will detect the error, but if you add the casts it is guaranteed to work.

In C they are compatible (6.2.7 Compatible type and composite type in N1570) if given in different translation unit - but in the example above they are in the same translation unit.

In both C and C++ a cast between A* and B* doesn't change any bits as far as I understand, whereas a cast to void* may imply that.

[henrikt-ma]

In both C and C++ a cast between A* and B* doesn't change any bits as far as I understand, whereas a cast to void* may imply that.

Exactly what bits could change if casting a record pointer to or from void*?

[henrikt-ma]

In C they are compatible (6.2.7 Compatible type and composite type in N1570) if given in different translation unit - but in the example above they are in the same translation unit.

I expect at carefully implemented external function to come with a header file corresponding to the implementation in the linked library. This header should be included in the compilation unit where where the external function is called, which means that the tools own representation of the record will coexist with the library's representation in the same translation unit. The only way that I see this can be handled without triggering compiler warnings is thus with a void* interface, and explicit casting to void* in the generated code as well as explicit casting from void* in the external library implementation.

[HansOlsson]

In both C and C++ a cast between A* and B* doesn't change any bits as far as I understand, whereas a cast to void* may imply that.

Exactly what bits could change if casting a record pointer to or from void*?

Potentially all of them. Different pointer types may have different internal representation (note that void* and char* should explicitly have same representation in C, but pointers to structs are not included).

However, you are supposed to get back the same bits when casting back from 'void*' - but that may change the bits.

But I don't see the relevance: I'm just trying to clarify existing behavior - which doesn't have any of these issues; and not get side-tracked and feature-creeped into adding more.

[henrikt-ma]

OK, I see that the question about the function signature could be discussed separately from the storage of the struct data. I'll open a separate issue about this (#3154).