-
Notifications
You must be signed in to change notification settings - Fork 168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Provide ModelicaUtilities.h file for people writing their own C external function #4484
Comments
One way to deal with the dependency on the MLS version would be to include some sort of assert that will fail unless a ( If we solve that part, we "only" need to agree upon what a correct content of ModelicaUtilities.h should look like. Once agreed, we should stop thinking of this file as a tool-specific file, and it should be expected (and probably even defined in the specification) that the file as provided by one tool should work with any other tool. At this point, I don't see why there should be any differences at all between tools, and it would be natural to include the file with every library, including ModelicaServices. The file included with ModelicaServices would then naturally become the de-facto standard file for tools makers and library authors to copy into their products. |
The central question here is why any tool would depend on ModelicaUtilities.h content that would not work with different tools? |
In #3867 we agreed that ModelicaUtilities.h is tool-specific, right? In that case it is no more applicable to compile external objects/functions with one tool-specific ModelicaUtilities.h and re-use the binary in different simulation tools. I also consider the removal of ModelicaUtilities.h as a breaking change in the sense that it should force a v5.0.0 instead of v4.1.0. |
One scenario is that tools might not yet fully support all utility functions of MLS 3.6, i.e. the later introduced functions ModelicaWarning or ModelicaDuplicateString. |
Making ModelicaUtilities.h tool-specific is a massive pain for library developers, if that decision was made I strongly suggest it should be un-done asap. Case in point, https://github.com/modelica-3rdparty/ExternalMedia It would be unreasonable to have to recompile and distribute ExternalMedia with a different ModelicaUtilities.h for every OS and tool combination, also considering that:
Unless ModelicaUtilities.h is standardized, the external functions interface is essentially broken and portability basically works "by chance" relying on undefined behavior. This needs to change. |
A tool that does not yet support all utility functions can provide the missing ones as stubs printing a message to the user "This feature is currently unsupported" or similar. |
Revert "refs modelica#3867: Remove LICENSE_ModelicaUtilities.txt" This reverts commit 7fe506f. Revert "refs modelica#3867: Remove ModelicaUtilities.h" This reverts commit 1bb1d40.
Revert "refs modelica#3867: Remove LICENSE_ModelicaUtilities.txt" This reverts commit 7fe506f. Revert "refs modelica#3867: Remove ModelicaUtilities.h" This reverts commit 1bb1d40.
Revert "refs modelica#3867: Remove LICENSE_ModelicaUtilities.txt" This reverts commit 7fe506f. Revert "refs modelica#3867: Remove ModelicaUtilities.h" This reverts commit 1bb1d40.
Excerpt from OpenModelica's void ModelicaFormatMessage(const char *string, ...) MODELICA_FORMATATTR_PRINTF;
/*
Output the message under the same format control as the C-function printf.
*/
void ModelicaVFormatMessage(const char *string, va_list args) MODELICA_FORMATATTR_VPRINTF;
/*
Output the message under the same format control as the C-function vprintf.
*/
MODELICA_NORETURN void ModelicaError(const char *string) MODELICA_NORETURNATTR;
/*
Output the error message string (no format control). This function
never returns to the calling function, but handles the error
similarly to an assert in the Modelica code.
*/ Excerpt from Dymola's DYMOLA_STATIC void ModelicaFormatMessage(const char *string, ...) MODELICA_FORMATATTR_PRINTF;
/*
Output the message under the same format control as the C-function printf.
*/
DYMOLA_STATIC void ModelicaVFormatMessage(const char *string, va_list args) MODELICA_FORMATATTR_VPRINTF;
/*
Output the message under the same format control as the C-function vprintf.
*/
MODELICA_NORETURN DYMOLA_STATIC void ModelicaError(const char *string) MODELICA_NORETURNATTR;
/*
Output the error message string (no format control). This function
never returns to the calling function, but handles the error
similarly to an assert in the Modelica code.
*/ As I understand (I'm not the expert here, I might be wrong), the tool-specific macros are needed when compiling the simulation executable with a given tool, which is OK. However, if I am writing a library with external functions that needs to call such utility functions, I obviously don't want to use any such tool-specific .h file in its source code, because it may not work with other tools. I just need a void ModelicaMessage (const char* string);
void ModelicaWarning (const char* string);
void ModelicaError(const char* string);
void ModelicaFormatMessage (const char* format , ...);
void ModelicaFormatWarning (const char* format , ...);
void ModelicaFormatError (const char* format , ...);
void ModelicaVFormatMessage (const char* format , va_list ap);
void ModelicaVFormatWarning (const char* format , va_list ap);
void ModelicaVFormatError (const char* format , va_list ap);
char* ModelicaAllocateString (size_t len);
char* ModelicaAllocateStringWithErrorReturn (size_t len);
char* ModelicaDuplicateString (const char* str );
char* ModelicaDuplicateStringWithErrorReturn (const char* str ); That's the public interface of those functions defined by the MLS, so if I use this for my external functions, I expect them to work in all tools. |
Revert "refs modelica#3867: Remove LICENSE_ModelicaUtilities.txt" This reverts commit 7fe506f. Revert "refs modelica#3867: Remove ModelicaUtilities.h" This reverts commit 1bb1d40.
Revert "refs modelica#3867: Remove LICENSE_ModelicaUtilities.txt" This reverts commit 7fe506f. Revert "refs modelica#3867: Remove ModelicaUtilities.h" This reverts commit 1bb1d40.
Last call on this issue before we go ahead with the 4.1.0 release. As stated in the MLS Section 12.9.6:
If I am writing my own external functions that need to call functions from ModelicaUtilities, I need to For clarity, please note that this ModelicaUtilities.h will not be the header file used by tool vendors to compile their own version of the ModelicaUtilities binaries. Each tool vendor will craft their own. This header file is meant to be used by everybody else, who needs to write Modelica functions that call the functions defined in such binaries. I would suggest that we include a bare-bones void ModelicaMessage (const char* string);
void ModelicaWarning (const char* string);
void ModelicaError(const char* string);
void ModelicaFormatMessage (const char* format , ...);
void ModelicaFormatWarning (const char* format , ...);
void ModelicaFormatError (const char* format , ...);
void ModelicaVFormatMessage (const char* format , va_list ap);
void ModelicaVFormatWarning (const char* format , va_list ap);
void ModelicaVFormatError (const char* format , va_list ap);
char* ModelicaAllocateString (size_t len);
char* ModelicaAllocateStringWithErrorReturn (size_t len);
char* ModelicaDuplicateString (const char* str );
char* ModelicaDuplicateStringWithErrorReturn (const char* str ); Two questions for the compiler experts, which I am not:
If the answer is YES, then we should add such a header file to the MSL, since I see no other reasonable place where to put it. If the answer is NO, then the next question is:
If for some reason it was not possible to have tool-neutral function signatures for the ModelicaUtilities functions, then I guess we'd have a big problem: basically there would be no such thing as tool-independent Modelica libraries as soon as they use external C code. @HansOlsson, @henrikt-ma, @adrpo, @gkurzbach, can you please comment on that? |
The answer depends on how freely tools have used the idea to provide their own version:
I don't know if we need the second option. However, there can also be other issues in external libraries, e.g., using C++ code and throwing exceptions as if that was safe in C-interfaces (it isn't). |
I did not change my mind. Let's revert that PR and keep the compatibility. We still can clarify for any later MSL release. I can not accept a replacement of the proper developed ModelicaUtilities.h by such a poor stub ModelicaUtilities.h. |
In my opinion we are way (way way way) past the point where we make non-critical bug-fix changes for 4.1.0. I would not call this a critical bug-fix, since the library works just fine, and this is a question about developing other libraries. |
Oh, a slight update of my answer. Some attributes of function declarations may in fact cause weird incompatibilities - specifically nonnull-attributes for pointers (as added in #1436 ) when using gcc/clang if the tool-vendor headers have attributes, and they don't compile their tool-libraries with |
Well, but not out of the box. See https://github.com/modelica/ModelicaStandardLibrary/blob/maint/4.1.x/Modelica/Resources/BuildProjects/readme.txt#L28-L30. As we also provide the binaries (as part of the released MSL) we need to get ModelicaUtilities.h from somewehere. Currently I take it from .CI/Test/ModelicaUtilities.h (https://github.com/modelica/ModelicaStandardLibrary/blob/maint/4.1.x/.CI/Test/ModelicaUtilities.h) which seems wrong. If we remove the binaries completely (and not even provide in a released version) then we could get rid of ModelicaUtilities.h. But that's not scheduled for v4.1.0. Hence, my preference is to restore the ModelicaUtilites.h and keep the compatbility. |
Could you elaborate why that seems wrong to you? To me, it seems good to have it somewhere that is not shipped with the library to customers (they don't need it), but still in the repo (the library developers need it to build binaries for a release). |
Before we continue the discussion, let me write some statements that we should all agree upon:
Please comment here if you believe that some of these statements are inaccurate. |
Now, consider the following scenario. John Doe is writing a Modelica package with external C functions that he is also developing from scratch; he wants to provided them to the package users by a compiled C library. Of course he wants the package to be usable on all Modelica tools. He is not affiliated with any tool vendor, so he cannot ask tool vendors to compiler the libraries for him. The question is: what ModelicaUtilites.h should John Doe include in the source code of his external C functions, so that once he's compiled them in a library, they will work in all tools?
|
I agree with that. We should carefully avoid to pass the message that the is no unique, tool-independent ModelicaUtilities.h header file, to be used by the John Does of the world. |
I respectfully disagree. Removing that file in #3871 had consequences that IMHO were not considered carefully enough. We're not talking about a bug fix, we are talking about something more fundamental. It's never too late for that. |
@beutlich I hope my last comments clarify this point. The fact that MSL libraries that are meant to be compiled by the tool vendors are present or not in the repo is totally irrelevant. |
Customers do need it.
Let's upscale this statement to its absurd consequences. Section 2.3.3 of the Modelica specification lists the Modelica language keywords. Among them there is the keyword Section 12.9.6 of the Modelica specification states that the ModelicaUtilities.h file should contain a function The fact that only a minority of Modelica users do write Modelica libraries which also contain C code does not make this fact less significant. Especially since some of these "low level" Modelica+C libraries are released to the public so that the wider Modelica community can benefit from them. ExternalMedia is an example. Tool vendors should not be allowed to customize ModelicaUtilities.h. If some OS-specific details such as dllexport declarations are needed, they should be contributed by means of #ifdefs to the one and only ModelicaUtilities.h that is present in the MSL. It may be a long way, but we should head in the direction in which no matter the tool, no matter if at compile-time or run-time, no matter what language the external code is written in, the functions declared in ModelicaUtilities.h should always work. Removing ModelicaUtilities.h from the MSL is a step in the wrong direction. |
The question is, what should I understand that the version of that file that we had in MSL 4.0.0 and that was removed by PR #3871 is what we need, or at least very close to it. So I'm definitely in favour of reverting #3878 and keep what we had in MSL 4.0.0. That's not a bug fix, it's a reversal of a questionable decision which was taken long time ago (when I was not responsible of MAP-Lib, BTW). We're still in time to reverse it, before causing potential future harm. @fedetftpolimi, if tool vendors wants to customize |
I fully agree with @casella and others: |
I'm not disagreeing with making that explicit - I was just trying to answer the questions about consequences. I also note that there's a subtle issue: ModelicaUtilities.h should (as a header file) containing function declarations, and that isn't clear at the moment (I will open a PR). However, one of the previous issues was about different versions of One solution for all of that is to not place Thus:
Generally agree, but to be clear: Annotations helping code analyzers/warnings are not "customizing" it as they don't impact the actual function calls. So, the current one should be ok, whereas dllimporting it or modifying the calling convention is not. |
The last point mean that I'm in favor of restoring the old |
No, they are never needed (the DYMOLA_STATIC is a bit different - it should certainly not be used when building a library.) However, they are a really good idea when compiling any code using these functions, as they document things and make it easy for the compiler to detect certain issues. In particular:
That is the documented behavior, so anyone using the functions can rely on those facts - it's just that it isn't (or wasn't) part of the C-standard, which mean that it has to be defined using various compiler extensions. |
Oh, and I noticed that the old ModelicaUtilities.h was in Modelica/Resources/C-Sources, not Include as I thought (right?). |
The issue I see is that we (MAP-Lib) ship (production) binaries for MSL that are created with a fle Modelicautilities.h with unknown origin or rather that orginates from a folder specific for CI tests.
See #4487. Note that this PR targets maint/4..x branch (not master, where there is the intention to remove binaries from the repo). |
Revert "refs modelica#3867: Remove LICENSE_ModelicaUtilities.txt" This reverts commit 7fe506f. Revert "refs modelica#3867: Remove ModelicaUtilities.h" This reverts commit 1bb1d40.
I can't find a reason why adding annotations to help code analyzers would break compatibility across Modelica tools, so from my perspective they are fine and thus I agree to restore the old ModelicaUtilities.h and not adding the barebones one. Do some tool vendor disagree? Let us know. |
BTW: A minor note regarding no-return - it has been added to C23, so we could use something like this:
|
Yes, we should add them. I prefer to also keep the existing defintions as fallback since I usually build with older compilers. |
I can understand that, but it seems weird at the moment. The special gcc-handling for below 4.8 (using attribute((noreturn)) for versions [2.8, 4.8) and nothing below 2.8) is only applied if defines indicate that it is C++11 and later, even though C++11 added a standard variant. That is only relevant if the compiler was broken. Digging deeper there seems to be such an issue with C++11 support in gcc 4.7.x, but neither in later versions nor in earlier ones. Later versions were corrected, and gcc 4.6 slightly predates C++11 and setting C++0x for that compiler doesn't seem to define __cplusplus to trigger this. Note that without C11, C23, C++11 there's still a fall-back for gcc 2.8 and later. And as far as I understand the latest gcc doesn't support the standard-attributes if specifying an old version of the standard, so I would assume the Added:
|
I'm glad to see some consensus building 😃. As I understand, there are potentially different
I would claim that the SSoT (besides the MLS Sect. 12.9.6, which is however "too bare-bones" to be usable) is 3., which sets the standard for all the John Does of the world. All other variants of that file need to be 100% compatible with it, possibly only adding some compiler optimizations or extra checks. ModelicaUtilities 2. is a bit tricky. I scanned the whole MSL for "ModelicaUtilities.h" and I found it in two categories of places:
So, when MAP-Lib compiles the MSL external function libraries with the CI, the path for ModelicaUtilities.h used by the C functions is set by the CI, whereas the path to the header file is hard-wired to Modelica/Resources/C-Sources when using the error() function in a user model. Some tools actually slightly modify 3. to get 1. (@HansOlsson doesn't like "customize" so I won't use that word 😃). For example, Dymola 2025x has a directory Sources, outside the MSL, which contains modified header and source files for ModelicaUtilities. The modifications do not change the interfaces, but add some extra information for the compiler and use tool-specific macros, e.g. Before #3871 was merged, 3. was found in So, I'm also in favour of merging #4487 as it is now. @HansOlsson comments that he would like to remove the I am also a bit unsure about the files found under Last, but not least, I would recommend to back-port all the changes we eventually make to maint/v4.1.x also onto master, since we want |
Me too.
For a long time Dymola used Visual Studio 2005 as a base-line for building these libraries. However, due to a breaking interface change we switched to 2015 (it is sort of possible to work around, but the code isn't fully working and it is truly horrible). Note that the gcc-issues are as old or older. The the noreturn-special attributes are for gcc 4.7 (released in 2011), and gcc before 2.8 (released in 1998). |
@casella #4496 addresses this.
@casella Either it can be seen as bug when ModelicaUtilities.h was removed. Alteratievely, the CPPFLAGS are uses/misused to inject the include directory of ModelicaUtitlities.h.
They are dinosaurs, but still maintained and used.
I will create a fresh PR targeting master branch: #4535 |
Revert "refs modelica#3867: Remove LICENSE_ModelicaUtilities.txt" This reverts commit 7fe506f. Revert "refs modelica#3867: Remove ModelicaUtilities.h" This reverts commit 1bb1d40.
* refs #4484: Restore ModelicaUtilities.h Revert "refs #3867: Remove LICENSE_ModelicaUtilities.txt" This reverts commit 7fe506f. Revert "refs #3867: Remove ModelicaUtilities.h" This reverts commit 1bb1d40. * Configure MODELICA_UTILITIES_INCLUDE_DIR for CMake * Modernize and simplify noreturn function attribute/specifier * Handle C23 * Remove fallback to legacy clang extension * Remove fallback to legacy gcc2/gcc3 extensions
@beutlich could you prepare a backport PR?
Yes, can do earliest by Sunday.
|
In #3867 there was a discussion about whether ModelicaUtilities.h in the MSL was meant to be tool-specific, which ended up in the removal of that file from the MSL.
If I understand correctly, the key argument for that made by @henrikt-ma:
Unfortunately, this leaves the following scenario without a clear solution. Suppose that I am writing a library (not the MSL, whose binaries are compiled by each tool vendor) that is meant to be usable by multiple tools (ca va sans dire), uses external functions, and is released with pre-compiled binaries (DLL/shared libraries). When I am writing the C code of external functions that need to call ModelicaUtilities stuff, I need to
#include "ModelicaUtilities.h"
to actually compile the code.The question is, where do I take that
ModelicaUtilities.h
file? Currently, I only see two feasible options:Option 1. is obviously dangerous from a portability point of view: what guarantee do I have that the resulting binary libraries are compatible with all Modelica tools? In fact, I may not even be allowed to do that by licencing agreements.
Option 2. is straightforward, but it seems a bit odd that we are not providing those definitions arlready packaged in a
ModelicaUtilitites.h
file somewhere.I think we should provide a version of
ModelicaUtilities.h
that only contains the function declarations (the interfaces / fingerprints), making it clear in the file header that this is not meant to be used by Modelica tools to manage their runtimes, but only to compile external functions in a tool-independent way.Technically speaking, this is part of the Modelica Language Specification. I guess that putting it in the MSL makes sense, provided we explain clearly what the purpose is.
Keeping @fedetftpolimi in the loop.
The text was updated successfully, but these errors were encountered: