-
Notifications
You must be signed in to change notification settings - Fork 560
scala-native 0.5.9 #4510
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
base: series/3.x
Are you sure you want to change the base?
scala-native 0.5.9 #4510
Conversation
#include <stddef.h> | ||
|
||
// we'll need POSIX for `sigaction`: | ||
#define _POSIX_C_SOURCE 1 |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It fails (on 0.5.9) with the same "incomplete type" and "implicit declaration" errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, conversation in #scala-native I realized that. Technically this should be defined before the inclusion of any other standard headers (so before stddef.h
) but I doubt it matters.
It would be an issue if i.e. sigaction depended on something in stddef.h that stddef.h only defines with _POSIX_C_SOURCE active.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it should be defined before the inclusion of practically anything anywhere (but we can't do that, it's not up to us). So it's not bulletproof, but I don't have a better idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This being a .c
file, the only earlier you can get is if someone passes -include
to the compiler. (This is related to why you only define _POSIX_C_SOURCE
in source files and not in header files: you can't be sure that your header will be included first.) TLDR I don't see why it's not bulletproof 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically this should be defined before the inclusion of any other standard headers
That is standards speak for "You will regret it if you do it any other way" ;-)
Stacktraces broke at least on ARM Linux (scala-native; 2.13 and 3); 2 examples: |
You should be able to just do the following: Or conversely like this: https://github.com/scala-native/scala-native/pull/4523/files |
Thank you for this PR. You know the CE build environment and there are plenty of log files. The code in By my understanding "-std=c11" gives one standard C ISO C. Defining "_POSIX_SOURCE" adds various elements marked by the Open Group (a.k.a POSIX) Defining "_GNU_SOURCE" adds more magic to the soup. Starting from a clean sheet of paper, "_POSIX_SOURCE" is preferable to most/many/well_a_few people. In this case, we are dealing with existing code which used, effectively, _GNU_SOURCE. I believe (I compared the file using -std=11, _POSIX_SOURCE, and _GNU_SOURCE using the clang/gcc -E Let me know if & how I can help. |
#ifdef CATS_EFFECT_SIGNAL_HELPER | ||
|
||
// we'll need POSIX for `sigaction`: | ||
#define _POSIX_C_SOURCE 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The macro with which I am familiar is "_POSIX_SOURCE". Is "_POSIX_C_SOURCE" here an alias for the former? If not, that may explain some lack of effect.
I just made a change in SN where I added guard code
so that the definition is idempotent.
Roughly (not compiled):
#if !defined(_GNU_SOURCE)
#define _GNU_SOURCE 1
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we'll need the #if
here too.
Apprently _POSIX_SOURCE
is obsolete, and equivalent to _POSIX_C_SOURCE 1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LeeTibbert you shouldn't need to guard it: https://godbolt.org/z/3WssGsE1r and my argument is that you shouldn't want to guard it. Getting an error when it's defined twice is good because it tells you someone is using it wrong (i.e. the only place it should be defined is at the very beginning of a source file ["translation unit"])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good discussion & good points.
People, especially in large projects with build files, specify compiler options
on the command line quite frequently. The Gell-Mann concept of "If it can be done
it will be done" applies here, I think.
Guarding it or not is, I believe, one of those points where it depends upon what one
wants/intends/can_live_with. Kind of like the reason that people bet on whether
a coin flip will be heads or tails. Personal preference & keeps things interesting.
Over time I have become much more defensive when writing code which gets compiled
anywhere off my machine. I have given up the idea that I control anything, even on that/those
machine(s).
I have become both reluctant to break user code and to give up a week of my time to an
interruption when I do. The determining question here is "Can a person compiling this
code possibly supply a value which conflicts?"
That is, if the code requires a methods introduced in an Open Group version 6, what happens if they
define a version, say 1, on the command line? In that case you are correct that the conflict should
be reported.
In this case, by my possibly mistaken reading, I believe that there is no current conflicting value,
even an empty replacement or version 1. Providing the guard will prevent a complaint in the
"innocent" situation of a user specifying a value. If at all possible and if it maintains correctness
of the code, user specified values should be respected.
I believe this is more important when the code specifies a value such as 1 and a user
wants to compile with a higher Open Group version, say 8, in order to gain a V8 definition
in another file. Why annoy the user by breaking then?
For me, I am much more concerned with the case where a user can specify a conflicting definition. Say they compile their entire project with "-std=gnu17". One would then have "_GNU_SOURCE" (and/or possibly a newer macro" defined at the same time as this file defining "_POSIX_C_SOURCE".
I believe, on hasty reading, that the <signal.h>
file included by signal_helper.c
will messily
detect and report this situation by having multiple definitions for some entry_points/functions.
I'm off to a another lesson in humility by chasing a Scala 2 `NullPointerException` in `ConcurrentHashMap`. I fear Cats Effects is probably affected by this bug. This discussion is much more interesting that that lesson ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@s5bug I this case I mostly agree with @LeeTibbert. Specifically, the scenario I'm guarding against is this: the user specifies -D_POSIX_C_SOURCE=2
(or later) when compiling; if we don't have the #ifndef
, then that causes a warning. However, we're actually fine with 2
(or later). As far as I understand, for that file, all we need is that we have some _POSIX_C_SOURCE
defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The determining question here is "Can a person compiling this code possibly supply a value which conflicts?"
My view is it is more useful to tell a user "hey you shouldn't be doing this" than let it go through. If SN wants to support people doing strange things with compiler flags, that's fine by me, but for cases outside of this one that require a _POSIX_C_SOURCE
other than 1
, at the very least rather than
#ifndef _POSIX_C_SOURCE
#define _POSIX_C_SOURCE 123456L
#endif
could we recommend
#if defined(_POSIX_C_SOURCE)
#if _POSIX_C_SOURCE < 123456L
#error ...
#endif
#else
#define _POSIX_C_SOURCE 123456L
#endif
somewhere?
Say they compile their entire project with "-std=gnu17"
My godbolt link shows this is not an issue: a warning is only thrown when a double-define occurs (GCC's private features.h
header does guard, even when -std=gnu...
is set).
Specifically, the scenario I'm guarding against is this: the user specifies
-D_POSIX_C_SOURCE=2
(or later) when compiling
I want to reiterate that the user should not be doing this. _POSIX_C_SOURCE
should be specified on a source-file level, not broadly across a whole compilation. If SN/CE wants to support this whole-compilation case, that's fine, I just want that decision to be made with full knowledge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to reiterate that the user should not be doing this. _POSIX_C_SOURCE should be specified on a source-file level, not broadly across a whole compilation.
I think it immediately says the developer there is POSIX inside. Something like the internal Scala Native posixlib
it makes sense for that compilation unit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to reiterate that the user should not be doing this. _POSIX_C_SOURCE should be specified on a source-file level, not broadly across a whole compilation.
I'm not convinced this is the case; man feature_test_macros
says this to me:
NOTE: In order to be effective, a feature test macro must be defined before including any header files. This can be done either in the compilation command (cc -DMACRO=value) or by defining the macro within the source code before including any headers.
It then goes on detailing various macros, including _POSIX_C_SOURCE
. So it seems fine to me.
Of course it's possible that there exists some other documentation or "best practices" which says otherwise. If so, I'd be happy to read that...
Do you think the errors in the log file have to do with 0.5.9 or with _Mumble_SOURCE? If the former, I will give things a closer look. I took a fast run thru the logs and could not |
Which errors, in which log file? |
Yes, there were some stack trace changes in 0.5.9. I believe that they had to do with "lazy loading". I do not want to be spreading falsehoods do to my not being up to date, but I thought stack traces were broken I looked at one of the cited log file and did not see a stack trace where the log said there was a failure. You might try again with _GNU_SOURCE to see if anything improves. I suspect you are seeing |
#ifdef CATS_EFFECT_SIGNAL_HELPER | ||
|
||
// we'll need POSIX for `sigaction`: | ||
#ifndef _POSIX_C_SOURCE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK _POSIX_C_SOURCE is newer than _POSIX_SOURCE. I believe that it
is supposed to have a value defined in the Open Group Standard. Something
much higher than 1.
Later: see version suggestion below and related discussion in CE Issue #4509
All the .h file code I was looking for used _POSIX_SOURCE, and not the former.
I guess the compiler will tell you/us if it can not find the symbols revealed by the
correct macro.
Yeah, I'm looking at scala-native/scala-native#4512, it seems possibly related. Some of our unittests fail at least on ARM Linux. All the failing tests depend on getting stacktraces. These same tests passed on ARM Linux with SN 0.5.8. I do not know yet the exact nature of the failures. |
The stacktrace thing is this: scala-native/scala-native#4526. |
Thank you for the SN Issue. Wojciech will see the discussion there. Do you happen to know what happens if you toggle |
@LeeTibbert We'll see: e40493b. |
I think I found the SN NullPointerException which was killing As always, yell if SN is misbehaving for CE. I may not be able to fix it, but I can listen & investigate. |
Overcome by events, logs for current commit are all green. |
s5bug: I like block you suggested about incompatible prior POSIX/Open_Group versions being specified.
People can and do specify POSIX versions outside the compilation unit: say on the command line by a build IMO, the basic problem is a project shipping files which have to be compiled outside of a known environment, say Hope all your coding is going well. |
WIP