Q: runtime pal.h declarations #37310

tmds · 2020-06-02T15:15:08Z

When updating versions of clang we had some compiler errors due to mismatch in declarations between pal.h and system headers (mainly: missing throw specifiers). We can fix these issues by making pal.h match with the system headers.

What is the rationale for having these declaration in pal.h, instead of including system headers?

cc @jkotas @janvorli @omajid

The text was updated successfully, but these errors were encountered:

omajid · 2020-06-02T15:22:07Z

For example, this is the sort of error we ran into with clang 10: #32837

jkotas · 2020-06-02T15:27:06Z

The PAL is trying to emulate Windows for historic reasons. It made the initial port to Unix easy. Definitions of number of functions on Windows are different from equivalent definitions on Unix. It means that we cannot just include system headers because of the definitions would collide.

Ideally, we would make this Windows emulator go away. A step in this direction would be to define PAL_STDCPP_COMPAT always and make sure that everything still builds and works. It would enable us to include system headers instead of duplicating selected parts of them.

janvorli · 2020-06-02T15:27:50Z

The runtime code on Unix never includes anything from the actual platform, just the pal.h that provides all the functions the runtime depends on. That includes emulated Windows APIs, Windows data types, some extra PAL APIs that don't match windows ones (names starting with PAL_) and some standard C functions like various printf / scanf etc.

The code in PAL implementation also includes the pal.h since it needs to use the Windows data types or define the functions declared in the pal.h. However, the pal.h is not included directly, but rather through palinternal.h that besides other things also handles conflicting function and type names between the Windows and Unix worlds. There are details on that in comments at the beginning of the palinternal.h.

The throw specifier case is kind of special as it needs to be on the function declarations in the pal.h and since these are included from both PAL and runtime, they need to match what the platform uses.

tmds · 2020-06-04T15:37:08Z

A step in this direction would be to define PAL_STDCPP_COMPAT always and make sure that everything still builds and works.

It would be interesting to give this a try to avoid these compiler issues in the future.

tmds · 2020-06-09T12:42:57Z

I experimented with this a bit, and came to a better understanding of what type things PAL deals with. 2 examples:

wchar_t is 2-byte utf-16 on Windows, and 4-byte 'unspecified encoding' on Linux. Since strings in managed .NET are UTF-16 at some point you need native functions that work with UTF-16, and those will be distinct, on non-Windows, from system functions that accept wchar_t.

There are some Windows-specific sal annotations on buffers using __in and __out:

runtime/src/coreclr/src/pal/inc/rt/sal.h

Lines 2658 to 2659 in db70edd

    
           #define __in                                                     _SAL1_Source_(__in, (), _In_) 
        
           #define __out                                                    _SAL1_Source_(__out, (), _Out_)

Some Linux headers use __in and __out as identifiers, so they can't be defined to disappear.

All such cases can be death with, though the effort is higher than what I had imagined.

yowl · 2021-12-30T21:46:57Z

I'm confused with the wchar_t problem on Linux. If you have PAL_STDCPP_COMPAT then you dont get the, for example, PAL_fwprintf implementation as its within a #ifndef PAL_STDCPP_COMPAT in pal.h What's the solution here, always substitute fwprintf and other wide char functions on Linux?

jkotas · 2021-12-31T02:22:34Z

#define fwprintf PAL_fwprintf is within #ifndef PAL_STDCPP_COMPAT. PAL_fwprintf is outside #ifndef PAL_STDCPP_COMPAT. You should be able to replace fwprintf with PAL_fwprintf to make things compile.

yowl · 2021-12-31T14:57:57Z

Thanks,

runtime/src/coreclr/pal/src/include/pal/palinternal.h

Line 335 in 07ed923

#include "mbusafecrt.h"

Uses size_t but at its include point that's been defined to DUMMY_size_t, the undef comes after, and hence:

In file included from /home/scott/github/runtimelab/src/coreclr/nativeresources/../pal/src/include/pal/palinternal.h:335:
  /home/scott/github/runtimelab/src/coreclr/pal/inc/mbusafecrt.h:48:40: error: unknown type name 'DUMMY_size_t'
  extern errno_t strcat_s( char* ioDest, size_t inDestBufferSize, const char* inSrc );
                                         ^
  /home/scott/github/runtimelab/src/coreclr/nativeresources/../pal/src/include/pal/palinternal.h:207:16: note: expanded from macro 'size_t'
  #define size_t DUMMY_size_t
                 ^

I tried adding to mbusafecrt.h:

#ifdef PAL_STDCPP_COMPAT
#undef size_t
#include <stddef.h>
#endif

which clears this one, but moves to the next:

  In file included from /home/scott/github/runtimelab/src/coreclr/nativeresources/resourcestring.cpp:6:
  In file included from /home/scott/github/runtimelab/src/coreclr/inc/utilcode.h:14:
  In file included from /home/scott/github/runtimelab/src/coreclr/inc/winwrap.h:41:
  In file included from /home/scott/github/runtimelab/src/coreclr/nativeresources/../pal/src/include/pal/malloc.hpp:22:
  In file included from /home/scott/github/runtimelab/src/coreclr/nativeresources/../pal/src/include/pal/corunix.hpp:23:
  In file included from /home/scott/github/runtimelab/src/coreclr/nativeresources/../pal/src/include/pal/palinternal.h:335:
  /home/scott/github/runtimelab/src/coreclr/pal/inc/mbusafecrt.h:53:16: error: declaration of 'strcat_s' has a different language linkage
  extern errno_t strcat_s( char* ioDest, size_t inDestBufferSize, const char* inSrc );
                 ^
  /home/scott/github/runtimelab/src/coreclr/pal/inc/rt/safecrt.h:666:17: note: previous definition is here
  errno_t __cdecl strcat_s(char *_Dst, size_t _SizeInBytes, const char *_Src)
                  ^

Comments in mbusafecrt.h indicates its for MacOS but its included unconditionally by palinternal.h. Maybe the right thing is not to include it for Linux ?

jkotas · 2021-12-31T15:09:13Z

Comments in mbusafecrt.h indicates its for MacOS

These comments are stale.

I think you can try putting the whole block of the #define ... DUMMY_.... defines in palinternal.h under #ifndef PAL_STDCPP_COMPAT.

yowl · 2021-12-31T16:57:42Z

Thanks again,

For the SAL stuff, would renaming __in/__out:

#ifndef PAL_STDCPP_COMPAT
#define __sal_in                                                     _SAL1_Source_(__in, (), _In_)
#define __sal_out                                                    _SAL1_Source_(__out, (), _Out_)
#else
#define __sal_in
#define __sal_out
#endif // !PAL_STDCPP_COMPAT

And then a bunch of search/replace where they are used be ok?

jkotas · 2021-12-31T18:38:58Z

I would expect it to be:

#define sal__in                                                     _SAL1_Source_(__in, (), _In_)
#define sal__out                                                    _SAL1_Source_(__out, (), _Out_)

#ifndef PAL_STDCPP_COMPAT
// These macros conflict with c++ headers.
#define __in    sal__in
#define __out   sal__out
#endif // !PAL_STDCPP_COMPAT

Otherwise, sounds good.

yowl · 2022-01-01T18:03:31Z

Happy New Year!

Ok, back to this, I've made a lot of changes, so hopefully I'm not straying off the path too much. What is the strategy regards the REG_xxx enums and ucontext.h. I see a few files including <sys/ucontext.h> so how should this problem be solved ?:

[ 50%] Building CXX object ToolBox/superpmi/superpmi-shim-simple/CMakeFiles/superpmi-shim-simple.dir/__/superpmi-shared/compileresult.cpp.o
  In file included from <built-in>:1:
  In file included from /home/scott/github/runtimelab/artifacts/obj/coreclr/Linux.x64.Debug/jit/CMakeFiles/clrjit_unix_x64_x64.dir/cmake_pch.hxx:5:
  In file included from /home/scott/github/runtimelab/src/coreclr/jit/jitpch.h:18:
  In file included from /home/scott/github/runtimelab/src/coreclr/jit/jit.h:673:
  In file included from /home/scott/github/runtimelab/src/coreclr/jit/target.h:114:
  /home/scott/github/runtimelab/src/coreclr/jit/register.h:42:1: error: redefinition of enumerator 'REG_RAX'
  REGDEF(RAX,     0, 0x0001, "rax"   )
  ^
  /home/scott/github/runtimelab/src/coreclr/jit/target.h:112:41: note: expanded from macro 'REGDEF'
  #define REGDEF(name, rnum, mask, sname) REG_##name = rnum,
                                          ^
  <scratch space>:70:1: note: expanded from here
  REG_RAX
  ^
  /usr/include/x86_64-linux-gnu/sys/ucontext.h:79:18: note: expanded from macro 'REG_RAX'
  # define REG_RAX        REG_RAX
                          ^
  /usr/include/x86_64-linux-gnu/sys/ucontext.h:78:3: note: previous definition is here
    REG_RAX,
    ^

jkotas · 2022-01-02T02:32:49Z

Is it possible to avoid including sys/ucontext.h when compiling the JIT?

Of course, it would be also possible to rename REG_... in the JIT to something else, but it feels like unnecessarily disruptive change.

NN--- · 2022-01-02T06:09:38Z

@jkotas You don't need to introduce new macros, there are already _In_ and _Out_ for the reasons mentioned above.
See: Understanding SAL.

Do you know which CMakeLists.txt should have PAL_STDCPP_COMPAT being added ?

POC with search and replacing
NN---@05d876e

yowl · 2022-01-02T17:14:36Z

Is it possible to avoid including sys/ucontext.h when compiling the JIT?

Yes, think so.

What should I be doing for FILE and PAL_FILE? I think I've got it wrong so want to check

  /home/scott/github/runtimelab/src/coreclr/jit/fgdiagnostic.cpp:665:24: error: assigning to 'FILE *' (aka '_IO_FILE *') from incompatible type 'PAL_FILE *' (aka '_PAL_FILE *')
          fgxFile      = _wfopen(filename, W("a+"));
                         ^~~~~~~~~~~~~~~~~~~~~~~~~~

Should _wfopen be returning PAL_FILE* ? Maybe I should be asking if consumers of the PAL headers, like this one, should they include palinternal.h or pal.h ?

jkotas · 2022-01-02T17:44:53Z

Should _wfopen be returning PAL_FILE* ?

Yes, PAL _wfopen should be returning PAL_FILE. I think this one should be fixed by changing the local variable type to PAL_FILE.

yowl · 2022-01-02T18:28:07Z

Thanks, last one for today:

_strnicmp seems to be a undefined for me, what provides this for non-ms platforms?

  /home/scott/github/runtimelab/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontextreader.cpp:314:17: error: use of undeclared identifier '_strnicmp'; did you mean 'strncmp'?
              if (_strnicmp(this->Hash, this->tocFile.GetElementPtr(curTOCIndex)->Hash, MD5_HASH_BUFFER_SIZE) == 0)
                  ^~~~~~~~~

jkotas · 2022-01-02T19:03:10Z

_strnicmp seems to be a undefined for me, what provides this for non-ms platforms?

It is in pal.h. It may need to be moved outside #ifndef PAL_STDCPP_COMPAT.

yowl · 2022-01-03T16:20:04Z

Thanks, the linux jit is now building at least, so the next one is the wasm jit which is different as it brings in the LLVM headers. Going a bit off topic so maybe I should carry on in the runtimelab issues. For errno what should I be doing for the LLVM header processing, it's undefed and there is PAL_error which is fine for the runtime(lab) source, but do I do what is done for !PAL_STDCPP_COMPAT:

#define errno  (*PAL_errno(PAL_get_caller))

?

Error at the moment is

  [ 84%] Building CXX object jit/CMakeFiles/clrjit_browser_wasm32_x64.dir/copyprop.cpp.o
  In file included from /home/scott/github/runtimelab/src/coreclr/jit/compiler.cpp:47:
  In file included from /home/scott/github/runtimelab/src/coreclr/jit/llvm.h:20:
  In file included from /usr/lib/llvm-9/include/llvm/IR/IRBuilder.h:18:
  In file included from /usr/lib/llvm-9/include/llvm/ADT/ArrayRef.h:12:
  In file included from /usr/lib/llvm-9/include/llvm/ADT/Hashing.h:48:
  In file included from /usr/lib/llvm-9/include/llvm/Support/Host.h:16:
  In file included from /usr/lib/llvm-9/include/llvm/ADT/StringMap.h:16:
  In file included from /usr/lib/llvm-9/include/llvm/ADT/StringRef.h:12:
  In file included from /usr/lib/llvm-9/include/llvm/ADT/STLExtras.h:19:
  In file included from /usr/lib/llvm-9/include/llvm/ADT/Optional.h:22:
  In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/memory:80:
  In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/unique_ptr.h:37:
  In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/tuple:39:
  In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/array:39:
  In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/stdexcept:39:
  In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/string:55:
  In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/basic_string.h:6493:
  /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/ext/string_conversions.h:63:27: error: use of undeclared identifier 'errno'
          _Save_errno() : _M_errno(errno) { errno = 0; }
                                   ^
  /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/cerrno:49:15: note: expanded from macro 'errno'
  #define errno errno
                ^

I did try this approach briefly, i.e. in pal.h just unconditionally

#undef errno
#define errno  (*PAL_errno(PAL_get_caller))

but:

  [ 50%] Building CXX object jit/CMakeFiles/clrjit_unix_x64_x64.dir/cmake_pch.hxx.pch
  In file included from <built-in>:1:
  In file included from /home/scott/github/runtimelab/artifacts/obj/coreclr/Linux.x64.Debug/ToolBox/superpmi/superpmi-shim-simple/CMakeFiles/superpmi-shim-simple.dir/cmake_pch.hxx:5:
  In file included from /home/scott/github/runtimelab/src/coreclr/ToolBox/superpmi/superpmi-shim-simple/../superpmi-shared/standardpch.h:70:
  In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/string:55:
  In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/basic_string.h:6493:
  /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/ext/string_conversions.h:63:27: error: called object type 'int' is not a function or function pointer
          _Save_errno() : _M_errno(errno) { errno = 0; }
                                   ^~~~~

jkotas · 2022-01-03T16:37:45Z

For errno what should I be doing for the LLVM header processing, it's undefed

Where is it undefed? I think we should be just picking up the OS definition of errno with PAL_STDCPP_COMPAT

yowl · 2022-01-03T16:48:37Z

At

runtime/src/coreclr/pal/src/include/pal/palinternal.h

Line 497 in 3cde4dc

#undef errno

Maybe I shouldn't have included that in compiler.h ...

jkotas · 2022-01-03T16:51:35Z

Maybe I shouldn't have included that in compiler.h

Agree.

yowl · 2022-01-03T17:20:41Z

Thanks, do you think all these places where I've included palinternal.h are wrong?:

Name	Line	Text	Path
methodcontextreader.cpp	16	#include "pal/palinternal.h"	\\wsl$\Ubuntu-20.04\home\scott\github\runtimelab\src\coreclr\ToolBox\superpmi\superpmi-shared
commandline.cpp	13	#include "pal/palinternal.h"	\\wsl$\Ubuntu-20.04\home\scott\github\runtimelab\src\coreclr\ToolBox\superpmi\mcs
utilcode.h	38	#include "pal/palinternal.h"	\\wsl$\Ubuntu-20.04\home\scott\github\runtimelab\src\coreclr\inc
fgdiagnostic.cpp	10	#include "pal/palinternal.h"	\\wsl$\Ubuntu-20.04\home\scott\github\runtimelab\src\coreclr\jit
utils.cpp	27	#include "pal/palinternal.h" // _strdup	\\wsl$\Ubuntu-20.04\home\scott\github\runtimelab\src\coreclr\jit

jkotas · 2022-01-03T17:50:38Z

I think palinternal.h should stay internal to the PAL. It should not be included anywhere else.

janvorli · 2022-01-03T18:28:20Z

Right, palinternal.h is supposed to be used only in the PAL itself.

yowl · 2022-01-03T21:06:51Z

Should _wfopen be returning PAL_FILE* ?

Yes, PAL _wfopen should be returning PAL_FILE. I think this one should be fixed by changing the local variable type to PAL_FILE.

Where is the actual definition of PAL_FILE I can just see the partial declaration in pal.h:

struct _PAL_FILE;
typedef struct _PAL_FILE PAL_FILE;

And a reference in cruntime.h:

/*++

struct PAL_FILE.
Used to mimic the behavior of windows.
fwrite under windows can set the ferror flag,
under BSD fwrite doesn't.
--*/
struct _FILE
{

But that's just _FILE, not actually defining the struct PAL_FILE What am i missing here? At dynamic link of the clrjit time it fails that it is undefined

   /home/scott/github/runtimelab/artifacts/bin/coreclr/Linux.x64.Debug/libclrjit_browser_wasm32_x64.so: error: symbol lookup error: undefined symbol: _Z5flogfP9_PAL_FILEPKcz (fatal)

jkotas · 2022-01-03T21:12:13Z

I can just see the partial declaration in pal.h:

The partial definition in pal.h that I see is:

struct _FILE;
typedef struct _FILE FILE;
typedef struct _FILE PAL_FILE;

struct _FILE is then defined in the PAL implementation.

yowl · 2022-01-03T22:56:44Z

Thanks, I'll move that outside of the #if

Improves dotnet#37310

* Fix pal_assert.h for PAL_STDCPP_COMPAT Improves #37310

janvorli · 2022-07-12T14:21:26Z

It seems that all the questions have been asnwered, closing the issue.

Dotnet-GitSync-Bot added area-PAL-coreclr untriaged New issue has not been triaged by the area owner labels Jun 2, 2020

janvorli removed the untriaged New issue has not been triaged by the area owner label Jun 22, 2020

janvorli added this to the 6.0.0 milestone Jun 22, 2020

janvorli modified the milestones: 6.0.0, 7.0.0 Jul 19, 2021

jkotas mentioned this issue Dec 30, 2021

[NativeAOT-LLVM] compiling for Linux dotnet/runtimelab#1797

Closed

NN--- mentioned this issue Jan 2, 2022

Use SAL2 _In_, _Out_ to prevent conflicts. #63261

Merged

kant2002 mentioned this issue Feb 1, 2022

Fix pal_assert.h when PAL_STDCPP_COMPAT defined #64631

Merged

kant2002 added a commit to kant2002/runtime that referenced this issue Feb 2, 2022

Compile coreclr\dlls folder with PAL_STDCPP_COMPAT

42df175

Improves dotnet#37310

kant2002 mentioned this issue Feb 2, 2022

Compile coreclr\dlls folder with PAL_STDCPP_COMPAT #64684

Closed

jkotas mentioned this issue Apr 2, 2022

Fix incorrect format string arguments #67464

Merged

kant2002 added a commit to kant2002/runtime that referenced this issue Apr 12, 2022

Fix pal_assert.h for PAL_STDCPP_COMPAT

863e46d

Improves dotnet#37310

AaronRobinsonMSFT pushed a commit that referenced this issue Apr 14, 2022

Fix pal_assert.h when PAL_STDCPP_COMPAT defined (#64631)

32e03eb

* Fix pal_assert.h for PAL_STDCPP_COMPAT Improves #37310

AaronRobinsonMSFT mentioned this issue Jun 4, 2022

Add explicit documentation about C/C++ feature usage rule #70239

Open

janvorli closed this as completed Jul 12, 2022

ghost locked as resolved and limited conversation to collaborators Aug 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Q: runtime pal.h declarations #37310

Q: runtime pal.h declarations #37310

tmds commented Jun 2, 2020

omajid commented Jun 2, 2020

jkotas commented Jun 2, 2020 •

edited

Loading

janvorli commented Jun 2, 2020

tmds commented Jun 4, 2020

tmds commented Jun 9, 2020

yowl commented Dec 30, 2021

jkotas commented Dec 31, 2021

yowl commented Dec 31, 2021

jkotas commented Dec 31, 2021

yowl commented Dec 31, 2021

jkotas commented Dec 31, 2021

yowl commented Jan 1, 2022

jkotas commented Jan 2, 2022

NN--- commented Jan 2, 2022

yowl commented Jan 2, 2022

jkotas commented Jan 2, 2022

yowl commented Jan 2, 2022 •

edited

Loading

jkotas commented Jan 2, 2022

yowl commented Jan 3, 2022

jkotas commented Jan 3, 2022

yowl commented Jan 3, 2022

jkotas commented Jan 3, 2022

yowl commented Jan 3, 2022

jkotas commented Jan 3, 2022

janvorli commented Jan 3, 2022

yowl commented Jan 3, 2022

jkotas commented Jan 3, 2022

yowl commented Jan 3, 2022

janvorli commented Jul 12, 2022

Q: runtime pal.h declarations #37310

Q: runtime pal.h declarations #37310

Comments

tmds commented Jun 2, 2020

omajid commented Jun 2, 2020

jkotas commented Jun 2, 2020 • edited Loading

janvorli commented Jun 2, 2020

tmds commented Jun 4, 2020

tmds commented Jun 9, 2020

yowl commented Dec 30, 2021

jkotas commented Dec 31, 2021

yowl commented Dec 31, 2021

jkotas commented Dec 31, 2021

yowl commented Dec 31, 2021

jkotas commented Dec 31, 2021

yowl commented Jan 1, 2022

jkotas commented Jan 2, 2022

NN--- commented Jan 2, 2022

yowl commented Jan 2, 2022

jkotas commented Jan 2, 2022

yowl commented Jan 2, 2022 • edited Loading

jkotas commented Jan 2, 2022

yowl commented Jan 3, 2022

jkotas commented Jan 3, 2022

yowl commented Jan 3, 2022

jkotas commented Jan 3, 2022

yowl commented Jan 3, 2022

jkotas commented Jan 3, 2022

janvorli commented Jan 3, 2022

yowl commented Jan 3, 2022

jkotas commented Jan 3, 2022

yowl commented Jan 3, 2022

janvorli commented Jul 12, 2022

jkotas commented Jun 2, 2020 •

edited

Loading

yowl commented Jan 2, 2022 •

edited

Loading