Skip to content
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

Fix C89 compliance #3787

Merged
merged 1 commit into from Apr 17, 2021
Merged

Fix C89 compliance #3787

merged 1 commit into from Apr 17, 2021

Conversation

beutlich
Copy link
Member

@beutlich beutlich commented Apr 7, 2021

POSIX is not availabe if C-Sources are compiled with -ansi (or -std=c89).

@beutlich beutlich added the L: C-Sources Issue addresses Modelica/Resources/C-Sources label Apr 7, 2021
@beutlich beutlich added this to the MSL4.1.0 milestone Apr 7, 2021
@beutlich beutlich self-assigned this Apr 7, 2021
Copy link
Member

@sjoelund sjoelund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The -ansi flag is not the same as being pure C89 without POSIX. POSIX functions are still available and there is no way to disable them on a standard GCC install on Linux as far as I know. That's why the _POSIX_VERSION macro is enabled. Almost nothing in the Linux system headers changes depending on the __STRICT_ANSI__ macro being present. Some macros change from being x ? y : z to if (x) y; else z;, disables inlining of some math functions. But nothing really major.

How did you find this in the first place?

@beutlich
Copy link
Member Author

beutlich commented Apr 8, 2021

How did you find this in the first place?

I found this when compiling C-sources with -std=c89 and got a compiler warning:

libtool: compile:  gcc "-DPACKAGE_NAME=\"Modelica Standard Library Tables\"" -DPACKAGE_TARNAME=\"libmodelicastandardtables\" -DPACKAGE_VERSION=\"4.0.0\" "-DPACKAGE_STRING=\"Modelica Standard Library Tables 4.0.0\"" -DPACKAGE_BUGREPORT=\"https://github.com/modelica/ModelicaStandardLibrary\" -DPACKAGE_URL=\"https://modelica.org\" -DPACKAGE=\"libmodelicastandardtables\" -DVERSION=\"4.0.0\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DHAVE_DIRENT_H=1 -DHAVE_LOCALE_H=1 -DHAVE_TIME_H=1 -I. -DDUMMY_FUNCTION_USERTAB=1 -DTABLE_SHARE=1 -g -O2 -fno-delete-null-pointer-checks -std=c89 -MT ../../C-Sources/ModelicaInternal.lo -MD -MP -MF ../../C-Sources/.deps/ModelicaInternal.Tpo -c ../../C-Sources/ModelicaInternal.c -o ../../C-Sources/ModelicaInternal.o
../../C-Sources/ModelicaInternal.c: In function ‘ModelicaInternal_getTime’:
../../C-Sources/ModelicaInternal.c:1339:12: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
     tlocal = localtime_r(&calendarTime, &tres); /* Time fields in local time zone */
            ^

localtime_r of time.h is not available when compiling with -std=c89.

@HansOlsson
Copy link
Contributor

How did you find this in the first place?

I found this when compiling C-sources with -std=c89 and got a compiler warning:

libtool: compile:  gcc "-DPACKAGE_NAME=\"Modelica Standard Library Tables\"" -DPACKAGE_TARNAME=\"libmodelicastandardtables\" -DPACKAGE_VERSION=\"4.0.0\" "-DPACKAGE_STRING=\"Modelica Standard Library Tables 4.0.0\"" -DPACKAGE_BUGREPORT=\"https://github.com/modelica/ModelicaStandardLibrary\" -DPACKAGE_URL=\"https://modelica.org\" -DPACKAGE=\"libmodelicastandardtables\" -DVERSION=\"4.0.0\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DHAVE_DIRENT_H=1 -DHAVE_LOCALE_H=1 -DHAVE_TIME_H=1 -I. -DDUMMY_FUNCTION_USERTAB=1 -DTABLE_SHARE=1 -g -O2 -fno-delete-null-pointer-checks -std=c89 -MT ../../C-Sources/ModelicaInternal.lo -MD -MP -MF ../../C-Sources/.deps/ModelicaInternal.Tpo -c ../../C-Sources/ModelicaInternal.c -o ../../C-Sources/ModelicaInternal.o
../../C-Sources/ModelicaInternal.c: In function ‘ModelicaInternal_getTime’:
../../C-Sources/ModelicaInternal.c:1339:12: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
     tlocal = localtime_r(&calendarTime, &tres); /* Time fields in local time zone */
            ^

localtime_r of time.h is not available when compiling with -std=c89.

We have to be clear regarding ISO (and ANSI) compliance. ANSI and ISO standards without a year normally refer to the latest released one (as far as I understand the previous standards are even superseded by the new ones), and as far as I understand the planned ISO standard nicknamed C23 (or C2x) is planning to add localtime_r. I don't know if it works with -std=c2x

Thus we should call this C89 compliance, not ISO Standard compliance; and ensure that it works with C23.

@sjoelund
Copy link
Member

sjoelund commented Apr 8, 2021

If you #include <unistd.h>, you will get some POSIX stuff available (but not things like putenv either; see -Werror=implicit-function-declaration). Most problematic is that it defines _POSIX_VERSION, which the standards say should only be available if that's the POSIX version available. It feels wrong to look for some GCC internal flag to be able to figure out if we actually have POSIX or not...

I guess the correct way on Linux/glibc would be to #include <features.h> instead of unistd.h. But then you need to know if that header is available (it's not on Mac). Also, on Mac localtime_r is available when STRICT_ANSI is active.

The following seems to work in both Linux and Mac (unix is not defined if STRICT_ANSI is given; could also be !defined(__STRICT_ANSI)):

#if (defined(__unix__) && (!defined(__linux__) || defined(unix))) || defined(__APPLE_CC__)
  #include <unistd.h>
#endif
#if !defined(_POSIX_) && defined(_POSIX_VERSION)
  #define _POSIX_ 1
#endif

That seems slightly better since we won't load unistd.h if -ansi is given

@sjoelund
Copy link
Member

sjoelund commented Apr 8, 2021

Or perhaps we could look for unix and not GLIBC with STRICT_ANSI. Might capture more OSes...

@beutlich beutlich changed the title Fix ISO Standard C compliance Fix C89 compliance Apr 8, 2021
@beutlich
Copy link
Member Author

beutlich commented Apr 8, 2021

What about defining _POSIX_C_SOURCE just as we already define _GNU_SOURCE?

See also https://stackoverflow.com/q/21867897/8520615.

@sjoelund
Copy link
Member

sjoelund commented Apr 8, 2021

What about defining _POSIX_C_SOURCE just as we already define _GNU_SOURCE?

See also https://stackoverflow.com/q/21867897/8520615.

That's also an option and would hopefully give us the correct value for _POSIX_VERSION from the headers

@beutlich
Copy link
Member Author

beutlich commented Apr 8, 2021

OK, it was easier than expected. Defining _GNU_SOURCE in ModelicaInternal.c fixes the issue with localtime_r and also enables realpath.

@beutlich beutlich requested a review from sjoelund April 8, 2021 18:27
Copy link
Member

@sjoelund sjoelund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still does not compile with C89 because zlib requires snprintf (C99 or #define _POSIX_C_SOURCE 200112L in gzwrite.c and gzlib.c)

@beutlich
Copy link
Member Author

beutlich commented Apr 9, 2021

This still does not compile with C89 because zlib requires snprintf (C99 or #define _POSIX_C_SOURCE 200112L in gzwrite.c and gzlib.c)

I disabled snprintf in case of C89. No need to worry, since gzwrite and gzlib are not used at all.

@sjoelund
Copy link
Member

sjoelund commented Apr 9, 2021

Then maybe we should remove gzlib and gzwrite sources? :)

Copy link
Contributor

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
I found a separate issue with C99-testing, but that deserves another PR.

@beutlich beutlich merged commit 62848d3 into modelica:master Apr 17, 2021
@beutlich beutlich deleted the fix-ansi-c branch April 17, 2021 10:13
beutlich added a commit to beutlich/ModelicaStandardLibrary that referenced this pull request Oct 14, 2022
Take care to not break modelica#3787 again.
@beutlich beutlich mentioned this pull request Oct 14, 2022
beutlich added a commit to beutlich/ModelicaStandardLibrary that referenced this pull request Aug 10, 2023
Take care to not break modelica#3787 again.
beutlich added a commit to beutlich/ModelicaStandardLibrary that referenced this pull request Aug 18, 2023
Take care to not break modelica#3787 again.
beutlich added a commit to beutlich/ModelicaStandardLibrary that referenced this pull request Aug 18, 2023
Take care to not break modelica#3787 again
beutlich added a commit to beutlich/ModelicaStandardLibrary that referenced this pull request Jan 10, 2024
Take care to not break modelica#3787 again.
beutlich added a commit to beutlich/ModelicaStandardLibrary that referenced this pull request Jan 10, 2024
Take care to not break modelica#3787 again
beutlich added a commit that referenced this pull request Jan 10, 2024
Take care to not break #3787 again.
@beutlich beutlich mentioned this pull request Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: C-Sources Issue addresses Modelica/Resources/C-Sources
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants