Fix for fcntl() flags definition #14751
Conversation
ciarmcom
added
the
release-type: patch
|
@tymoteuszblochmobica, thank you for your changes. |
| @@ -63,17 +64,36 @@ typedef unsigned int gid_t; ///< Group ID | |||
| /* Flags for open() and fcntl(GETFL/SETFL) | |||
| * At present, fcntl only supports reading and writing O_NONBLOCK | |||
| */ | |||
| #ifndef O_RDONLY | |||
There was a problem hiding this comment.
I am trying to find a reasoning we had it previously as undef. Didn't we have fcntl in one of the toolchains (I bet it was IAR back then) ? If this is tested with both toolchains we support, it should be good.
Avoiding redefinition errors (either this PR or as it was would result with the same effect), however the previous one ignored the values set by elsewhere and forcing our retarget value (undef + set). This PR is changing it to (ifndef then set).
@kjbracey-arm Would you know if the proposed way is acceptable ?
There was a problem hiding this comment.
I think ifndef is better for errno because it maintains binary compatibility with the C library. There will be some C library functions setting errno (but not many in this embedded environment - usually it's the porting layer), and they would be using the values from the toolchain header.
I'm not sure what the original justification for the forced undefine was. Consistency of values between toolchains?
Comment says this, but it's not totally illuminating:
* Note also that ARMCC errno.h defines some symbol values differently from
* the GCC_ARM/IAR/standard POSIX definitions. The definitions guard against
* this and future changes by changing the symbol definition as shown below.
The implication being that the values below were matching GNU+IAR?
Switching values now (for ARMC only?) would potentially introduce binary compatibility issues with other binary blobs. But I doubt that would matter. The sort of things using binary blobs don't use these values - Wifi drivers would be using NSAPI error codes.
So I don't have a problem with this change, conceptually.
The main issue is that if doing ifdef, then this file really must make sure it has included the toolchain file that would define these to get the same result every time.
Otherwise
#include "mbed_retarget.h"
and
#include <sys/something>
#include "mbed_retarget.h"
might give you two different values in different compilation units. (Maybe that was the point of the undefine?)
There was a problem hiding this comment.
Thanks for commenting on this Kevin.
Most of the values below match GCC's newlib values but not all of them; some are different. It is what motivated the change. For example, EADDRINUSE is equal to 112 on Mbed OS but is set to 98 in newlib.
This has caused us issues when implementing a POSIX compatible socket layer on top of Mbed OS as the values between the system header and mbed header were not matching.
The inclusion of errno.h and fcntl.h should cover us as defines are reused from here.
The last difficulty is to find if a value defined in this file doesn't collide with another errno value defined in the system header. I think we can use a constexpr and static_assert (maybe just in the cpp file to not impact build time) to validates that these errno values are distinct:
constexpr bool validate_errno_values() {
constexpr int values[] {
EPERM,
ENOENT,
ESRCH
/* other errno values ... */
};
constexpr int count = sizeof(values) / sizeof(values[0]);
for (int i = 0; i < count; ++i) {
for (int j = i + 1; j < count; ++j) {
if (i == j) {
continue;
}
if (values[i] == values[j]) {
return false;
}
}
}
return true;
}
static_assert(validate_errno_values() == true, "Not all errno values are unique");@tymoteuszblochmobica Have you validated the change with ARMCC ?
There was a problem hiding this comment.
Cute, but you don't need to go that far. Just make a switch statement listing each value. That will give a compile error for duplicates.
(EAGAIN and EWOULDBLOCK are allowed to be duplicates, others aren't, I believe).
There was a problem hiding this comment.
Aha, I completely forgot about the switch statement 🤦 . That should work too and be faster.
|
This pull request has automatically been marked as stale because it has had no recent activity. @pan-, please complete review of the changes to move the PR forward. Thank you for your contributions. |
tymoteuszblochmobica
force-pushed
the
POSIX
branch
from
84c3acc
to
dfca7ec
Compare
|
I added switch to verify duplicates at compile time. |
| #define O_RDONLY 0 ///< Open for reading | ||
| #endif |
There was a problem hiding this comment.
Which one was redefined ? That could be terrible if some are equals to others.
There was a problem hiding this comment.
Mmm, it is worth sanity checking you haven't made a mismatched set.
The same switch trick for all the O_ defines is basically sufficient. You might want to also add
static_assert((O_ACCMODE & (O_NONBLOCK|O_APPEND|O_CREAT|O_TRUNC|O_EXCL|O_BINARY)) == 0, "O_ACCMODE clash")
| case EOWNERDEAD: | ||
| break; | ||
| case ENOTRECOVERABLE: | ||
| break; |
There was a problem hiding this comment.
I think we should check for equality too as pointed out by Kevin.
can you add static asserts for them ?
There was a problem hiding this comment.
Eh? What did I suggest? The switch is testing for equality. (A comment explaining that would be a good idea...)
Not sure the breaks are needed - wouldn't expect any compiler to complain about "fallthrough" for directly adjacent cases.
There was a problem hiding this comment.
I should have precise that I was speaking about EAGAIN and EWOULDBLOCK. It is not mandatory to have them equal thought.
In such case we need a second switch case where EAGAIN is replaced with EWOULDBLOCK.
There was a problem hiding this comment.
I believe you can simply do:
#if EAGAIN != EWOULDBLOCK
case EAGAIN:
#endif
case EWOULDBLOCK:
POSIX says the defines have to be usable in #if directives.
tymoteuszblochmobica
force-pushed
the
POSIX
branch
from
dfca7ec
to
18179f2
Compare
|
Definition conflicts First time defined ERESTART 85 ELIBSCN 85 |
|
@tymoteuszblochmobica I don't follow the last comment. All comments were resolved, the switch function works and this is ready for integration or there is still some redefinition errors? |
|
@0xc0170 I think @tymoteuszblochmobica just mention the define values that were changed in this PR. |
|
CI started |
Jenkins CI Test : ❌ FAILEDBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
|
Please review failures for Gcc, they are related |
Jenkins CI Test : ❌ FAILEDBuild Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
|
@tymoteuszblochmobica have you tried locally, any of example to build with Gcc Arm? The second run confirmed the failures. |
|
I tried locally with mbed-os and added main. There was no errors. |
Disable setting own values of flags for fcntl() function if they are defined
tymoteuszblochmobica
force-pushed
the
POSIX
branch
from
18179f2
to
f1663e7
Compare
Jenkins CI Test : ❌ FAILEDBuild Number: 3 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
Jenkins CI Test : ✔️ SUCCESSBuild Number: 4 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
|
Hi |
| #define ELIBEXEC 83 /* Cannot exec a shared library directly */ | ||
| #undef EILSEQ | ||
| #endif | ||
| #ifndef EILSEQ |
There was a problem hiding this comment.
Got issue with this one only for IAR...
Seems EILSEQ is wrongly defined?
$ grep -r EILSEQ *
arm/inc/c/errno.h: #define EILSEQ 36
mbedmain
removed
release-type: patch
Summary of changes
Disable override errno erro value in mbed_retarget.h
Disable setting own values of flags for fcntl() function if they are defined
Impact of changes
No impact
Migration actions required
Not needed
Documentation
None
Pull request type
Test results
Reviewers
@pan-
@ ATmobica