ZOSOpenTools / meta

Meta repository to tie together the various underlying z/OS Open Source tools repositories here
https://zosopentools.github.io/meta/
Apache License 2.0
37 stars 25 forks source link

How to handle omvs functions not conforming to (the latest) posix standards? #77

Closed mfsysprog closed 1 year ago

mfsysprog commented 1 year ago

When porting nano I came across the issue that realpath is not compliant to the current posix standard on omvs. Posix.1-2008 descibes that it should accept the second parameter as a NULL value and that it should allocate a buffer using malloc in that case (https://pubs.opengroup.org/onlinepubs/9699919799/). For z/os it does not conform to that standard (https://www.ibm.com/docs/en/zos/2.4.0?topic=functions-realpath-resolve-path-name) and will not accept the second parm as a NULL (giving an EINV response). I guess this was the expected behaviour in some earlier posix standard. This leads to the following questions:

Also, I was reading about library interposing, in linux this can be done using LD_PRELOAD. Does omvs support the same concept?

MikeFultonDev commented 1 year ago

My opinion would be that we add the function into zoslib so everyone porting can benefit and also raise an enhancement request to z/OS for them to improve the function going forward. That provides a practical as well as strategic approach to making progress.

mfsysprog commented 1 year ago

I had the same idea, but I'm not sure if it is possible in this case (without changing the nano code), since realpath is a syscall. I'm not sure if it is possible to override a syscall with a function from a shared library with the same name and still be able to call the original syscall from that function.

mfsysprog commented 1 year ago

Ah, it can be done using the syscall function as it seems

mfsysprog commented 1 year ago

hmm, don't see a syscall function on omvs... So I guess it is possible to intercept the system call be providing a library function with the same name, but can you then still call the original syscall?

mfsysprog commented 1 year ago

I might even be mistaken that realpath is a syscall. It is listed as a C/C++ library function. Would it be possible to override realpath in zoslib and load the actual realpath using dlsym as real_realpath?

mfsysprog commented 1 year ago

So far it seems there is no syscall available and also RTLD_NEXT is not supported, so I don't think there is a way to actually 'wrap' the realpath function, at least not a way that I can find.

mfsysprog commented 1 year ago

I think I may have found a way, that is to call BPX4RPH from the 'wrapper' function. I still have to test the idea further, but it looks promising. One issue is that it works in ebcdic, so the path and the returned path should be translated if the calling application is running in ascii. Is the idea of ports that they will always run in ascii, or should a solution be bimodal? Also, will it always be 64 bit, or should it also support 31 bit callers? Even if I get it to work, I wouldn't want to include it in zoslib, perhaps a zoscompatlib or something would be a better idea for 'enhancing' syscalls until omvs fixes it themselves?

mfsysprog commented 1 year ago

note to self: https://github.com/libuv/libuv/blob/64669fdd8de8fce478e6ce1f7c954905f682c75b/src/unix/fs.c#L759

MikeFultonDev commented 1 year ago

That’s great to hear. Hope it pans out. We are building everything 64-bit and ASCII. You could make it bimodal if you want. I have to think of the best way to do that but one way would be to check if the _BPX_AUTOCVT option is on. @IgorTodorovskiIBM probably has a better way. To convert from EBCDIC to ASCII you can use iconv

mfsysprog commented 1 year ago

I can use __isASCII() to check if the thread is running in ascii. I'm having some issues building at the moment. Did the perl environment change? It seems the version in /etc/profile is not there anymore.

MikeFultonDev commented 1 year ago

Hi

Hm. Nothing changed that I know of but I don’t use /etc/ for Perl - I use zopen download to pull down my tools including Perl

On Sat, Nov 26, 2022 at 09:51 mfsysprog @.***> wrote:

I can use __isASCII() to check if the thread is running in ascii. I'm having some issues building at the moment. Did the perl environment change? It seems the version in /etc/profile is not there anymore.

— Reply to this email directly, view it on GitHub https://github.com/ZOSOpenTools/meta/issues/77#issuecomment-1328087896, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOSFFEMIJOCLBYMZS77OUELWKJEYJANCNFSM6AAAAAASF7TFOU . You are receiving this because you commented.Message ID: @.***>

IgorTodorovskiIBM commented 1 year ago

You could use the technique we use to override LE's open as in here: https://github.com/ibmruntimes/zoslib/blob/zopen/include/fcntl.h#L38 and https://github.com/ibmruntimes/zoslib/blob/zopen/src/zos-io.cc#L720

Right now we're building all our tools, including zoslib in 64-bit ASCII mode.

mfsysprog commented 1 year ago

Yes, that should work! I'll see if I can add realpath that way. Note to self: offset for realpath at 884 for the debug.

@MikeFultonDev; When I add zoslib as a dependency it will download to zopen/prod, so if I clone zoslibport and add the change there that would be the easiest way to create a change in zoslib locally?

I have succesfully tested the concept by adding below code in one of the c files:

int pathmax_size(const char* path) {
  int pathmax;

  pathmax = pathconf(path, _PC_PATH_MAX);

  if (pathmax == -1)
    pathmax = PATH_MAX;

  return pathmax;

}

char *realpath(const char *restrict path, char *restrict resolved_path)
{
   char *ebcdic_path = malloc(strlen(path) + 1);
   strcpy(ebcdic_path, path);
   if (__isASCII()){
      __a2e_s(ebcdic_path);
   }
   int path_length=strlen(path);
   if (resolved_path == NULL){
      int len = pathmax_size(path);
      resolved_path = malloc(len + 1);
   }
   int path_resolved_length=strlen(resolved_path);
   int rv, rc, reason;
   BPX4RPH(&path_length, ebcdic_path, &path_resolved_length, resolved_path, &rv,
 &rc, &reason);
   free(ebcdic_path);
   if (rv == -1) {
      errno = rc;
      return NULL;
   }
   if (__isASCII()){
      __e2a_s(resolved_path);
   }
   return resolved_path;
}
mfsysprog commented 1 year ago

Hi Hm. Nothing changed that I know of but I don’t use /etc/ for Perl - I use zopen download to pull down my tools including Perl I made a change to Makefile.am, so it triggered the automake/autoconf stuff. But avoided that issue by adding the test code directly to one of the existing c files so I didn't need to change the Makefile.am

IgorTodorovskiIBM commented 1 year ago

Hi, usually this is the process I follow when I want to test a change I made in zoslib with another dependency. 1) clone zoslibport and build it with zopen build 2) Make my changes in the zoslibport/zoslib dir and then rebuild with zopen build 3) Once installed, I move the ~/zopen/prod/zoslib-zopen* dir to ~/zopen/prod/zoslib so that it gets picked up by the dependent projects. 4) Rebuild my project that depends on zoslib 5) If all is ok, I create a PR against https://github.com/ibmruntimes/zoslib/tree/zopen

mfsysprog commented 1 year ago

I'm getting an error when trying to build zoslibport:

CMake Error at /mfsyspg/zopen/prod/CMake-heads.v3.24.2.20221116_114743.zos/share/cmake-3.24/Modules/CMakeTestCXXCompiler.cmake:62 (message):
  The C++ compiler

    "/bin/xlclang++"

  is not able to compile a simple test program.

  It fails with the following output:

    Change Dir: /mfsyspg/zoslibport/zoslib/CMakeFiles/CMakeTmp

    Run Build Command(s):/mfsyspg/zopen/prod/make-4.3.20221111_144539.zos/bin/make -f Makefile cmTC_a5b5d/fast && /mfsyspg/zopen/prod/make-4.3.20221111_144539.zos/bin/make  -f CMakeFiles/cmTC_a5b5d.dir/build.make CMakeFiles/cmTC_a5b5d.dir/build
    make[1]: Entering directory '/mfsyspg/zoslibport/zoslib/CMakeFiles/CMakeTmp'
    Building CXX object CMakeFiles/cmTC_a5b5d.dir/testCXXCompiler.cxx.o
    /bin/xlclang++   -+ -qascii -qnocsect -qenum=int  -o CMakeFiles/cmTC_a5b5d.dir/testCXXCompiler.cxx.o -c /mfsyspg/zoslibport/zoslib/CMakeFiles/CMakeTmp/testCXXCompiler.cxx
    Linking CXX executable cmTC_a5b5d
    /mfsyspg/zopen/prod/CMake-heads.v3.24.2.20221116_114743.zos/bin/cmake -E cmake_link_script CMakeFiles/cmTC_a5b5d.dir/link.txt --verbose=1
    /bin/xlclang++ -+ -qascii -qnocsect -qenum=int  CMakeFiles/cmTC_a5b5d.dir/testCXXCompiler.cxx.o -o cmTC_a5b5d 
    FSUM3010 Specify a file with the correct suffix (.C, .hh, .i, .c, .i, .s, .o, .x, .p, .I, or .a), or a corresponding data set name, instead of -o./cmTC_a5b5d.
    make[1]: *** [CMakeFiles/cmTC_a5b5d.dir/build.make:99: cmTC_a5b5d] Error 1
    make[1]: Leaving directory '/mfsyspg/zoslibport/zoslib/CMakeFiles/CMakeTmp'
    make: *** [Makefile:127: cmTC_a5b5d/fast] Error 2

Any idea why this is happening? Is my setup wrong somewhere?

mfsysprog commented 1 year ago

https://www.ibm.com/docs/en/SSCH7P_3.8.0/python.pdf seems to have the answer: If you see the following errors, FSUM3010 Specify a file with the correct suffix (.C, .hh, .i, .c, .i, .s, .o, .x, .p, .I, or .a), or a corresponding data set name.... export the following environment variables: export _CC_CCMODE=1 export _CXX_CCMODE=1 export _C89_CCMODE=1 export _CC_EXTRA_ARGS=1 export _CXX_EXTRA_ARGS=1 export _C89_EXTRA_ARGS=1

IgorTodorovskiIBM commented 1 year ago

Yes, that should fix it! That's why we added it to zopen's common.inc file: https://github.com/ZOSOpenTools/utils/blob/main/bin/lib/common.inc#L35

mfsysprog commented 1 year ago

Ah thanks, I hadn't noticed the utils repo yet.

IgorTodorovskiIBM commented 1 year ago

If you clone utils and set your PATH to utils/bin, then you should be able to do a zopen build -v in your zoslibport directory.

mfsysprog commented 1 year ago

I must be doing something wrong, I just cannot get it to call the zoslib version of realpath I implemented. I've basically added this to zos-bpx.cc:

char *__realpath_orig(const char __restrict__ *path, char __restrict__ *resolved_path) asm("@@A00187");

char *__realpath_extended(const char __restrict__ *path, char __restrict__ *resolved_path){
   if (resolved_path == NULL){
      resolved_path = (char*) malloc(pathmax_size(path) + 1);
   }
   return __realpath_orig(path, resolved_path);
}

When I call __realpath_extended from the nano code directly it works flawlessly, so the zoslib library is correctly linked.

When I use realpath from the nano code, it goes to the default realpath. These are the changed I made to the include/stdlib.h in zoslib:

diff --git a/include/stdlib.h b/include/stdlib.h
index bf6b4fa..f02ae7d 100644
--- a/include/stdlib.h
+++ b/include/stdlib.h
@@ -15,6 +15,7 @@
 #if defined(__cplusplus)
 extern "C" {
 #endif
+__Z_EXPORT extern char *__realpath_extended(const char * __restrict__, char * __restrict__);
 __Z_EXPORT int __mkstemp_ascii(char*);
 #if defined(__cplusplus)
 };
@@ -22,15 +23,22 @@ __Z_EXPORT int __mkstemp_ascii(char*);

 #if defined(ZOSLIB_OVERRIDE_CLIB) || defined(ZOSLIB_OVERRIDE_CLIB_STDLIB)

+#undef realpath
+#define realpath __realpath_replaced
 #undef mkstemp
 #define mkstemp __mkstemp_replaced
 #include_next <stdlib.h>
-#undef mkstemp 
+#undef mkstemp
+#undef realpath

 #if defined(__cplusplus)
 extern "C" {
 #endif

+/**
+ * Same as original realpath, but this allocates a buffer if second parm is NULL as defined in Posix.1-2008
+ */
+__Z_EXPORT extern char *realpath(const char * __restrict__, char * __restrict__) asm("__realpath_extended");
 /**
  * Same as C mkstemp but tags fd as ASCII (819)
  */

My understanding is, that the original realpath prototype will be renamed to __realpath_replaced and that the new realpath definition will be 'diverted' to __realpath_extended. Any idea why this is not working?

mfsysprog commented 1 year ago

This is the original definition of realpath from stdlib.h:

        char        *realpath(const char * __restrict__,
                                    char * __restrict__) __THROW;
IgorTodorovskiIBM commented 1 year ago

Hmm, I don't see anything obviously wrong with the changes you made. One thing to confirm is that the zoslib -I includes are being added. I think in one case I noticed CPPFLAGS was ignored by a project, but it still built because C only warns for implicit definitions.

Another thing to check is are you seeing some of the other overridden symbols like open and mkstemps are changed to the __open_ascii/__mkstemp_ascii symbols. You should be able to use the nm utility to check this. Although you'll probably have to remove the -Wl,EDIT(NO) option or change it to YES to get symbols.

mfsysprog commented 1 year ago

ah ok, I already looked with nm and I do see the __realpath_extended, but did not see the overriden ones. Where whould I have to remove the -Wl,EDIT(NO)?

IgorTodorovskiIBM commented 1 year ago

Just to confirm, you added zoslib as a dependency in your buildenv and ran it through zopen build?

You could temporarily modify utils/bin/lib/zopen-build to remove -Wl,EDIT(NO). It might make sense to not set this when the buildtype is debug (zopen build -b debug) so that we can check the symbols.

mfsysprog commented 1 year ago

yes, I added the dependency and I can see with nm that __realpath_extended is in the nano executable

IgorTodorovskiIBM commented 1 year ago

I wonder if -DZOSLIB_OVERRIDE_CLIB=1 is being passed, which is part of the CPPFLAGS

IgorTodorovskiIBM commented 1 year ago

You're seeing __realpath_extended after you directly called it or via the override?

mfsysprog commented 1 year ago

I think it is, I can see that the open function is overridden. I also tested this by removing the if defined(ZOSLIB_OVERRIDE_CLIB), but it didn't make a difference.

mfsysprog commented 1 year ago

I see that it is included in the module with nm and I can call it directly, but not via the override

IgorTodorovskiIBM commented 1 year ago

Hmm, it might be worthwhile to check the preprocessor output via the -E option to see if it's actually getting replaced

mfsysprog commented 1 year ago

I can't get the zoslib build to use the -E option, it works for my nano build. Probably something with the cmake stuff. I'm thinking that it is perhaps the asterisk in realpath that is the issue. The #define realpath __realpath_replace might not match realpath, but I cannot find a way to create a #define realpath __realpath_replace to test that theory

IgorTodorovskiIBM commented 1 year ago

Hmm, I just tried doing the macro hackery on a small test and with -E it does show it as being replaced:

...
        char *__realpath_replaced(const char * ,
                                    char * );

I noticed on my system it's picking up stdlib from /usr/include/le/stdlib.h and it doesn't have __THROW at the end.

I wonder if you manually compile the one nano source file that uses realpath and do a -E on it, does it get replaced for you?

mfsysprog commented 1 year ago

I found my error, I only copied the lib directory, I forgot to copy the changed stdlib.h

mfsysprog commented 1 year ago

I created a pull request for the enhanced realpath: https://github.com/ibmruntimes/zoslib/pull/7