Re: [PATCH] Add VxWorks fixincludes hack, open posix API
Hi Rasmus,
> On 17 Dec 2021, at 13:49, Rasmus Villemoes wrote:
>
> In the end, we ended up adding a two-argument overload for C++ only, as
> this is/was only relevant for getting libstdc++ (more specifically, the
> new filesystem abstraction stuff) to build. That is, we added
>
> +#if __GNUC__ > 6 && defined(__cplusplus)
> +extern "C++" {
> +extern int open (const char *name, int flags);
> +}
> +#endif
> +
>
> to fcntl.h, and added a trivial definition of that (which calls the
> tree-argument form with a 0 for mode) to the OS build. I guess it could
> also have been an inline.
I experimented with the inline track (patch attached) and we're
having good results with it. It builds and test results we get
are on par with those we had with the varargs approach.
Works better for you? Thanks again for raising the point.
Cheers,
Olivier
2021-01-10 Olivier Hainque
Rasmus Villemoes
* inclhack.def (vxworks_posix_open): New hack.
* tests/base/fcntl.h: Update.
* fixincl.x: Regenerate.
0003-Add-VxWorks-fixincludes-hack-open-posix-API.patch
Description: Binary data
Re: [PATCH] Add VxWorks fixincludes hack, open posix API
> On 17 Dec 2021, at 15:33, Rasmus Villemoes wrote:
>
> On 17/12/2021 15.12, Olivier Hainque wrote:
>> Hi Rasmus
>>
>>> On 17 Dec 2021, at 13:49, Rasmus Villemoes
>>> wrote:
>>
>>> I'm not sure what to do. But this patch will definitely break our build
>>> - primarily because we've done a private workaround.
>>
>> I don't think we can reasonably cope with private changes
>> to system headers.
>
> Of course not. And I'm more than willing to undo that private change if
> a suitable fix can be worked out.
>
>> Can't you just undo your workaround and use this (very simple)
>> "fix" instead?
>
> No, because as I explained, the open() implementation on vxworks 5.5
> must not be called as a two-arg function with garbage in r5.
> Do you have
> access to any of the kernel source code for the other vxworks versions
> with a three-arg-only open() in fcntl.h? If not, how can you know that
> those kernels do not make use of the mode argument even if O_CREAT is
> not in flags? (For example, I could actually imagine an implementation
> where non-zero mode would imply O_CREAT. Then 'open("foo", O_RDWR)'
> could result in foo being created with a more-or-less random mode, where
> it should have resulted in ENOENT.)
>
> I'm sure libstdc++ builds with this change, as I said I had the same
> thing initially, but after looking at the open() implementation we were
> worried about the implications.
Ah, I see. Sorry, I misunderstood the point you were making.
Let me check and think about it some more.
Cheers,
Olivier
Re: [PATCH] Add VxWorks fixincludes hack, open posix API
On 17/12/2021 15.12, Olivier Hainque wrote:
> Hi Rasmus
>
>> On 17 Dec 2021, at 13:49, Rasmus Villemoes
>> wrote:
>
>> I'm not sure what to do. But this patch will definitely break our build
>> - primarily because we've done a private workaround.
>
> I don't think we can reasonably cope with private changes
> to system headers.
Of course not. And I'm more than willing to undo that private change if
a suitable fix can be worked out.
> Can't you just undo your workaround and use this (very simple)
> "fix" instead?
No, because as I explained, the open() implementation on vxworks 5.5
must not be called as a two-arg function with garbage in r5. Do you have
access to any of the kernel source code for the other vxworks versions
with a three-arg-only open() in fcntl.h? If not, how can you know that
those kernels do not make use of the mode argument even if O_CREAT is
not in flags? (For example, I could actually imagine an implementation
where non-zero mode would imply O_CREAT. Then 'open("foo", O_RDWR)'
could result in foo being created with a more-or-less random mode, where
it should have resulted in ENOENT.)
I'm sure libstdc++ builds with this change, as I said I had the same
thing initially, but after looking at the open() implementation we were
worried about the implications.
> Otherwise, add a local patch to your tree to remove this fix?
Always an option, of course.
Rasmus
Re: [PATCH] Add VxWorks fixincludes hack, open posix API
Hi Rasmus > On 17 Dec 2021, at 13:49, Rasmus Villemoes wrote: > I'm not sure what to do. But this patch will definitely break our build > - primarily because we've done a private workaround. I don't think we can reasonably cope with private changes to system headers. Can't you just undo your workaround and use this (very simple) "fix" instead? Otherwise, add a local patch to your tree to remove this fix? Thanks in advance, Olivier
Re: [PATCH] Add VxWorks fixincludes hack, open posix API
On 17/12/2021 13.01, Olivier Hainque wrote:
> Hello,
>
> The attach patch adds a fixincludes "hack" for VxWorks
> to expose a more flexible (varargs) function prototype for 'open',
> able to accept calls with 2 or 3 arguments as we observe
> during libraries builds for powerpc vxworks 6.9.
>
> We have been using this for a while in-house. I could
> still observe related failures with mainline sources without
> the change and get a complete successful with it (plus a couple
> of others).
>
> Also bootstrapped and regression tested ok for x86_64-linux,
> just in case.
>
> Ok to commit?
I had proposed more-or-less the same patch to my customer, just being
done to the vxworks 5.5 headers directly.
We then looked at the part of the kernel source we do have available
(and which gets rebuilt when they build their OS kernel). That happens
to include the open() implementation. We could modify the open()
implementation accordingly, fetching a mode argument iff there's O_CREAT
in flags, and otherwise setting it to 0. Unfortunately, looking down the
call tree from open(), it turns out that there are some weird uses of
the mode argument even when O_CREAT was not given (I don't remember the
details, but I think one case was looking at a non-mode bit, S_IFDIR. It
was somewhat hard to follow due to several levels of indirect calls, and
I don't even think we had the code for all the possible callbacks).
Unconditionally fetching the third argument [1] isn't really an option
either as that would be garbage for any two-argument call site.
In the end, we ended up adding a two-argument overload for C++ only, as
this is/was only relevant for getting libstdc++ (more specifically, the
new filesystem abstraction stuff) to build. That is, we added
+#if __GNUC__ > 6 && defined(__cplusplus)
+extern "C++" {
+extern int open (const char *name, int flags);
+}
+#endif
+
to fcntl.h, and added a trivial definition of that (which calls the
tree-argument form with a 0 for mode) to the OS build. I guess it could
also have been an inline.
Another option we considered was inspired the fortified definition of
open() in glibc, which checks that when flags is compile-time known and
O_CREAT is there, there is a mode argument. We would not do that check,
but simply use __va_arg_pack_len() to decide whether the caller had
given a mode argument, and if not, pass a 0. That would also provide C
code with the ability to use both two- and three-arg open().
I'm not sure what to do. But this patch will definitely break our build
- primarily because we've done a private workaround.
Rasmus
[1] Instead of making the open() definition variadic, we could probably
also play a trick like renaming it at C level, and then re-renaming at
asm level, i.e. something like
-int open(const char *path, int flags, int mode) {
+int vxworks_open(const char *path, int flags, int mode) asm("open") {
...
but that would still make mode contain garbage when called with two
arguments.
