State record with integer variable is not handled correctly in external function

[casella]

Download the ExternalMedia 3.3.1 library from the Github release page: https://github.com/modelica-3rdparty/ExternalMedia/releases/download/v3.3.1/ExternalMedia_v3.3.1.zip

If I run ExternalMedia.Test.TestMedium.TestState with the latest nightly build on Windows, I get this warning

TestState_functions.c:52:65: warning: incompatible pointer types passing 'ExternalMedia_Test_TestMedium_TestState_Medium_ThermodynamicState *' to parameter of type 'ExternalThermodynamicState *' (aka 'struct ExternalThermodynamicState *') [-Wincompatible-pointer-types]
  TwoPhaseMedium_setState_ph_C_impl(_p_ext, _h_ext, _phase_ext, &_state_ext, MMC_STRINGDATA(MMC_REFSTRINGLIT(tmp1)), MMC_STRINGDATA(MMC_REFSTRINGLIT(tmp2)), MMC_STRINGDATA(MMC_REFSTRINGLIT(tmp3)));
                                                                ^~~~~~~~~~~
D:/Lavoro/Modelica/ExternalMedia 3.3.1/Resources/Include\externalmedialib.h:170:121: note: passing argument to parameter 'state' here
        EXTERNALMEDIA_EXPORT void TwoPhaseMedium_setState_ph_C_impl(double p, double h, int phase, ExternalThermodynamicState *state, const char *mediumName, const char *libraryName, const char *substanceName);

The simulation runs successfully, but the result displayed in OMEdit for state.phase, which is supposed to be an integer value of one, is instead zero at time zero, 1.4e14 during the simulation, and 6.2e12 at the final instant. Clearly something goes wrong as that integer is returned from the external function and passed back to the Modelica environment.

@mahge, can you please check? The results in Linux may be different, but probably still plain wrong.

[mahge]

This probably a result of a package constant not being evaluated by the external function handling. See this comment from @perost in #8738.

I guess. in this specific case, your library (the dll) was not found by omc (maybe you did not copy it to the right place as discussed in #8738). In which case the model would reach code generation with the variable unevaluated. See if you get see a warning from omc saying that the dll is not found. OR you can put the dll in the right location and see if this error is still there.

[perost]

This probably a result of a package constant not being evaluated by the external function handling. See this comment from @perost in #8738.

No, state is an output parameter of the function and not a package constant.

The issue seems to be two things. The first is that the names don't match, i.e. we generate a struct called ExternalMedia_Test_TestMedium_TestState_Medium_ThermodynamicState while the library uses ExternalThermodynamicState, and it seems this alone is enough to make the compiler say they are incompatible.

The other issue is that they actually are incompatible, becuase we use modelica_real and modelica_integer in the structs we generate while the external code uses double and int as defined in the specification. modelica_integer is defined as long (Linux 64-bit) or long long (Windows 64-bit), which is twice as large as int on those platforms.

So if you have a model such as:

record R
  Real x;
  Integer y;
  Integer z;
end R;

function f
  output R r;
  external "C" f_impl(r) annotation(Include="#include \"f.h\"");
end f;

model M
  R r;
  Integer y; // Need this or the record won't be output in the result for some reason
equation
  r = f();
  y = r.y;
end M;

and then the external C code:

typedef struct ExternalR {
  double x;
  int y;
  int z;
} ExternalR;

void f_impl(ExternalR *r)
{
  r->x = 1.0;
  r->y = 2;
  r->z = 3;
}

The simulating this gives r.y = 2 and r.z = -256 for me on Linux 64-bit. If I change int to long in the external C code then everything is fine, but that's of course not correct according to the specification.

[mahge]

Ahh. I saw the same message when the dll was not found in #8738 and together with you comment assumed it was caused by it. It is too much to hope for :(.

Now that I read the description properly I can see that I was way off since the message I got was not just a warning but an actual error as well. Thanks for taking the time to check.

I guess this is something to be discussed properly because we can not just change modelica_integer to int without some consequences.

[perost]

I guess this is something to be discussed properly because we can not just change modelica_integer to int without some consequences.

Yeah, the only practical solution I can think of would be to generate separate declarations for external structs and convert them when passing them back and forth.

It's odd that no one has noticed this before but I guess external records aren't that common, and padding sometimes hides the issue too (the reason why the record has two integers in my example).

[casella]

Yeah, the only practical solution I can think of would be to generate separate declarations for external structs and convert them when passing them back and forth.

@mahge do you think you can handle this?

It's odd that no one has noticed this before but I guess external records aren't that common

Indeed, I'm only aware of this happening in ExternalMedia. Which has ben around for 15 yrs, but we've not been able to deal with it in omc until recently.

and padding sometimes hides the issue too (the reason why the record has two integers in my example).

This is really diabolic :)

[casella]

@perost we need to sort this out to support ExternalMedia in OMC, which is something many people are asking (including myself). Are you now working on this? It would be good to have at least a workaround to make some progress.

Thanks!

[perost]

@perost we need to sort this out to support ExternalMedia in OMC, which is something many people are asking (including myself). Are you now working on this? It would be good to have at least a workaround to make some progress.

Thanks!

No, I assumed @mahge would look into it since he's worked on the code generation for records in the past.

[casella]

@mahge can you do that? It turns out this is also needed to run Buildings, so it is really top priority. Thanks!

[casella]

Devmeeting discussion: @mahge cleaned up the source code to use modelica_int everywhere. In order to ensure external function compilance, this is then defined as int. There is a small drawback, that number above 2^31 cannot be handled in 64-bit systems, but I guess nobody needs that, the only reason to switch to 64-bit is to access more memory for really large models.

There could be some problem if one writes 10000000000, which is an Integer constant that doesn't fit 32-bits. However, in all practical cases, 10000000000 means 10000000000.0, so we just issue a warning and convert the large integer literal into a Real Modelica variable.

[casella]

@mahge do you have a tentative date for the merging of the fixes with Modelica_int into master?

[mahge]

@casella The PR #9064 is now ready. I had to double check the failing tests to make sure there are no surprises. We can merge it and hopefully everything goes fine. If not we can come back to it and see if there are any alternatives.

[casella]

@mahge unfortunately we had to revert #9064 as it caused many other issues on Windows 64 bits, since Windows interprets "int" as 32 bits in that case.

I guess we'll need some alternative 😅

[mahge]

No problem 🙂 It was not meant to be. It was actually surprising that it worked for as much as it did. It is not theoretically impossible but I did not expect it to work at first. We at least know exactly what would break if we ever need to do it again. I hope not.

We will go with the alternative. With some help from the NF in finding these records, I think we can do it without having to traverse or generate more that we need.

This story concerns a fox that tries to eat grapes from a vine but cannot reach them. Rather than admit defeat, he states they are undesirable. The expression "sour grapes" originated from this fable.

[casella]

Nondum matura est! (it is not yet ripe) 😄

[bilderbuchi]

The simulation runs successfully, but the result displayed in OMEdit for state.phase, which is supposed to be an integer value of one, is instead zero at time zero, 1.4e14 during the simulation, and 6.2e12 at the final instant. Clearly something goes wrong as that integer is returned from the external function and passed back to the Modelica environment.

Minor update: with current ExternalMedia master modelica-3rdparty/ExternalMedia@97fb739 and OM 1.19.2, state.phase shows the correct value 1 at the beginning and end, and (unchanged) 1.4e14 in between.

[casella]

@bilderbuchi the current implementation uses 32 bits instead of 64 (or the other way round), so the result is totally unpredictable until we have a proper solution. @mahge is working on that.

[casella]

Update after today's devmeeting: @mahge thought about it. Completely rewriting the code generation to use int instead of modelica_int is a massive endeavour, which may take too much time.

A reasonable solution is to flag external functions in the frontend, and then use special code generation for them only.

@perost, @mahge, please agree on a solution that is not too cumbersome to implement, so we can finally get ExternalMedia to work in OMEdit.

Thanks!

[casella]

Outcome of today's discussion at the OSMC devmeeting:

• @mahge has a working solution, where records in external functions are handled differently than records in regular Modelica functions. It works for records only containing simple types, no arrays, no records
• it is good enough to support ExternalMedia as it is today, with pure fluids only. It won't work in the future when ExternalMedia is expanded to handle mixtures
• expanding it further would require a lot more work duplicating existing functionality, which doesn't really make sense
• an alternative solution proposed by @adrpo is to handle MetaModelica and Modelica functions differently. MetaModelica functions will continue using modelica_int, while Modelica functions could use int. This needs to be tested to make sure there are no borderline cases causing trouble.

Plan for action

1. @mahge pushes whatever he has now. It shouldn't break existing stuff, unless there are libraries around that use records in external functions, including arrays and sub-records. There could some of them, but I'm not aware of any.
2. we test this with ExternalMedia and it works fine, then we have a working solution for 1.20.0
3. after branching off 1.20.0, @mahge can roll back his commits and try the solution proposed by @adrpo, and we can test it thoroughly. If it works fine, then we keep it for 1.21.0

[mahge]

@mahge has a working solution, where records in external functions are handled differently than records in regular Modelica functions. It works for records only containing simple types, no arrays, no records

Working for the case in the ticket (including the MWE posted by Per) but there are still broken things in the testsuite. At least as of right now.

it is good enough to support ExternalMedia as it is today, with pure fluids only. It won't work in the future when ExternalMedia is expanded to handle mixtures

It is good enough to support ExternaMedia as it is today. Assuming it is not using anything more complicated than simple types in records.

@mahge pushes whatever he has now. It shouldn't break existing stuff, unless there are libraries around that use records in external functions, including arrays and sub-records. There could some of them, but I'm not aware of any.

It breaks some things we have in the testsuite. Last check had 578 tests failing. Changes like this cannot be done just for that model. The changes we make affect everything sometimes, so I have to follow up and fix the broken parts.

after branching off 1.20.0, @mahge can roll back his commits and try the solution proposed by @adrpo, and we can test it thoroughly. If it works fine, then we keep it for 1.21.0

When do you plan to have a release of ExternaMedia? If it not absolutely urgent I want to go back to the previous fix and test out @adrpo's suggestions before we merge this one. If that works, it is probably better.

[casella]

@mahge has a working solution, where records in external functions are handled differently than records in regular Modelica functions. It works for records only containing simple types, no arrays, no records

Working for the case in the ticket (including the MWE posted by Per) but there are still broken things in the testsuite. At least as of right now.

Ouch, I didn't get it. This changes the scenario significantly, because it could still take a lot of time to fix them.

it is good enough to support ExternalMedia as it is today, with pure fluids only. It won't work in the future when ExternalMedia is expanded to handle mixtures

It is good enough to support ExternaMedia as it is today. Assuming it is not using anything more complicated than simple types in records.

It doesn't.

@mahge pushes whatever he has now. It shouldn't break existing stuff, unless there are libraries around that use records in external functions, including arrays and sub-records. There could some of them, but I'm not aware of any.

It breaks some things we have in the testsuite. Last check had 578 tests failing. Changes like this cannot be done just for that model. The changes we make affect everything sometimes, so I have to follow up and fix the broken parts.

after branching off 1.20.0, @mahge can roll back his commits and try the solution proposed by @adrpo, and we can test it thoroughly. If it works fine, then we keep it for 1.21.0

When do you plan to have a release of ExternaMedia? If it not absolutely urgent I want to go back to the previous fix and test out @adrpo's suggestions before we merge this one. If that works, it is probably better.

@mahge I totally agree. ExternalMedia is ready to be released, whether or not this issue works in OpenModelica. Support for ExternalMedia can wait a couple more weeks for 1.20.0 to be branched off. If then it works, we could have a 1.20.1 with that fix only in one or two months, so it is available in a released version.

Thanks!

[casella]

I confirm that I am getting the correct results with ExternalMedia on the latest nightly builds. Thanks @mahge!

We will proceed ASAP with a new release of the library and add it to the library testsuite.