ModelicaStandardLibrary icon indicating copy to clipboard operation
ModelicaStandardLibrary copied to clipboard

External ModelicaStandardTables lack const qualifiers for array inputs (Modelica Language Specification version 3.5)

Open rfranke opened this issue 3 years ago • 10 comments

The Modelica spec 3.5 finally added const qualifiers for the mapping of array inputs to external C functions -- see 3.5 vs 3.4 ModelicaStandardTables should follow the standard to not break the compilation of generated code.

For instance ModelicaStandardTables_CombiTable2D_init2 is declared without const qualifier for the table argument in ModelicaStandardTables.h:

MODELICA_EXPORT void* ModelicaStandardTables_CombiTable2D_init2(_In_z_ const char* fileName,
                                                _In_z_ const char* tableName,
                                                _In_ double* table, size_t nRow,
                                                size_t nColumn, int smoothness,
                                                int extrapolation,
                                                int verbose) MODELICA_NONNULLATTR;

Modelica.Blocks.Types.ExternalCombiTable2D calls this function with the array input table though:

class ExternalCombiTable2D
  "External object of 2-dim. table defined by matrix"
  extends ExternalObject;

  function constructor "Initialize 2-dim. table defined by matrix"
    extends Modelica.Icons.Function;
    input String tableName "Table name";
    input String fileName "File name";
    input Real table[:, :];
    input Modelica.Blocks.Types.Smoothness smoothness;
    input Modelica.Blocks.Types.Extrapolation extrapolation=Modelica.Blocks.Types.Extrapolation.LastTwoPoints;
    input Boolean verboseRead=true "= true: Print info message; = false: No info message";
    output ExternalCombiTable2D externalCombiTable2D;
  external "C" externalCombiTable2D = ModelicaStandardTables_CombiTable2D_init2(
          fileName,
          tableName,
          table,
          size(table, 1),
          size(table, 2),
          smoothness,
          extrapolation,
          verboseRead) annotation (IncludeDirectory="modelica://Modelica/Resources/C-Sources", Include="#include \"ModelicaStandardTables.h\"", Library={"ModelicaStandardTables", "ModelicaIO", "ModelicaMatIO", "zlib"});
  end constructor;

  ...
end ExternalCombiTable2D;

rfranke avatar Jun 12 '21 08:06 rfranke

Can you confirm if #3700 solves the issue.

beutlich avatar Jun 12 '21 14:06 beutlich

#3700 looks good. Modelica 3.5 brings mostly clarifications for MSL. When does MSL plan to be based on it?

rfranke avatar Jun 12 '21 21:06 rfranke

Following discussions under #3700, the best solution might be to revert e40361763bdf818b9d6bbeaf8bf1f28212b3c80c. It was intended as improvement, but what does it improve? It hinders the adoption of Modelica 3.5 instead. This is bad as 3.5 is intended to clarify things and improve interoperability, not to break anything. Thus better revert e40361763bdf818b9d6bbeaf8bf1f28212b3c80c?

rfranke avatar Jun 16 '21 04:06 rfranke

The Include-annotations are essential for detecting mismatches between expected and actual implementation. I really prefer having them, not least in the MSL as a way of demonstrating how external functions should be handled.

henrikt-ma avatar Jun 16 '21 05:06 henrikt-ma

In general you are right. But how to get out of the dead end in this specific case?

rfranke avatar Jun 16 '21 06:06 rfranke

By just sitting and waiting for language version 3.5 to reach MSL.

henrikt-ma avatar Jun 16 '21 06:06 henrikt-ma

Would this then be MSL 5.0 due to a breaking change with include annotations?

rfranke avatar Jun 16 '21 06:06 rfranke

This is not for me to judge, but in general I don't believe in a black and white picture of backward compatibility. I am very positive about semantic versioning in general, but to stay relevant I think one must be prepared to allow one or two relatively harmless backward incompatible changes without bumping major version once in a while. Otherwise, major has to be bumped so often that it becomes pointless to have minor releases, and then most of the value of having semantic versioning is lost.

In this case, I think one can also distinguish between tool compatibility and library use compatibility. If I understand this correctly, introducing the Include-annotations can cause problems with tools, not for libraries that depend on MSL. When doing semantic versioning of a library, I don't think tool compatibility should be considered, and in this case this is captured well enough anyway by requiring language version 3.5.

henrikt-ma avatar Jun 16 '21 06:06 henrikt-ma

If we must have the Include annotations, then the MSL should be updated to allow MLS 3.5 as soon as possible. We patched away the Include annotations for OpenModelica and everything now works fine for both code generators (the one using const arrays like in MLS 3.5 and the one that does not like in MLS 3.4). We of course do not verify that the signatures are correct, but not working at all would be worse.

sjoelund avatar Jun 21 '21 10:06 sjoelund

The signatures should be correct for the widely tested MSL. It is good that the latest Modelica spec finally addresses issue #1955: "Changed inputs to C functions to be const-correct, section 12.9.1.2.". This took 5 years, which might be acceptable due to low criticality. It is bad though that commit e40361763bdf818b9d6bbeaf8bf1f28212b3c80c converts an uncritical unsolved issue to a critical unsolved issue. Then 5+ years is a really long time, compared to otherwise fast moving technologies everywhere around.

rfranke avatar Jun 23 '21 05:06 rfranke