Conflicts with assert.h & CHAR_BIT when PAL_STDCPP_COMPAT is defined

[k15tfu]

Hi!

I gonna play with Profiler API on Linux but firstly want to understand the project structure and what is supposed to be used when building profiler.so. In other issues you refer to https://github.com/Microsoft/clr-samples repo to show how it works.

Looking into ReJITEnterLeaveHooks/build.sh I realized that the following directories are required:

• $CORECLR_PATH/src/inc -- I think it's mostly for cor.h & corhdr.h
• $CORECLR_PATH/src/pal/prebuilt/inc -- for pre-generated headers from .idl/.xml files
• $CORECLR_PATH/src/pal/inc/rt -- it looks like stubs for Windows-specific headers
• $CORECLR_PATH/src/pal/inc -- common stuff used by rt/*

Are those ^^ correct?

Finally, when I set up my project to use them I got non-obvious inclusion of your pal.h file: boost/test/included/unit_test.hpp -> ... -> string -> cassert -> src/pal/inc/rt/assert.h -> src/pal/inc/rt/palrt.h -> src/pal/inc/pal.h. In addition assert() was defined to _ASSERTE() macro despite the fact I set PAL_STDCPP_COMPAT.

I cannot exclude src/pal/inc/rt from include-directories because it also contains ntimage.h & win*.h files and I don't want to pass the path to C/C++ headers to be sure the system-defined assert.h will be chosen.

1. If src/pal/inc/rt is really for Windows-specific headers why you placed C standard header there? Looks like you already have src/pal/inc/rt/cpp directory with C/C++ non-/standard headers.
2. If rt/assert.h cannot be moved to rt/cpp directory, can we implement it as follows:

//
// ===========================================================================
// File:  assert.h
//
// ===========================================================================
// dummy assert.h for PAL

#ifdef PAL_STDCPP_COMPAT
#include_next <assert.h>
#else
#include "palrt.h"
#endif

3. Why you define CHAR_BIT and other standard constants in pal.h#L239 without checks? Should we include <limits.h> after line 46 and then place these constants into #ifdef - #endif block?

[k15tfu]

Friendly ping

[janvorli]

If src/pal/inc/rt is really for Windows-specific headers why you placed C standard header there? Looks like you already have src/pal/inc/rt/cpp directory with C/C++ non-/standard headers.

I think this reason is lost in history. It seems it would logically make sense to move the assert.h to the cpp subfolder.

In addition assert() was defined to _ASSERTE() macro despite the fact I set PAL_STDCPP_COMPAT.

I am not sure if VS debugger that uses PAL_STDCPP_COMPAT and the PAL itself actually doesn't want to use the _ASSERTE macro. Your case is different as you want to use just the headers and not the pal library.

Why you define CHAR_BIT and other standard constants in pal.h#L239 without checks? Should we include <limits.h> after line 46 and then place these constants into #ifdef - #endif block?

As I have said before, the pal headers were not meant to be used for anything else than coreclr build originally. The PAL_STDCPP_COMPAT was added and the related changes were made just to allow building of the VS debugger. It was not done as enabling of coexistence with any platform specific headers. I would be fine with a PR that would make the change you've suggested, provided the constants end up defined to the same values we have in the file.

It is kind of unfortunate that the profiler sample includes the PAL headers from coreclr. If it carried copies of just the stuff it needs, you would not hit issues like this. I am pretty sure it would be only a small subset of the stuff that it ends up pulling in now.

[k15tfu]

I think this reason is lost in history. It seems it would logically make sense to move the assert.h to the cpp subfolder.

Okay, I can try to move it to cpp folder.

I would be fine with a PR that would make the change you've suggested, provided the constants end up defined to the same values we have in the file.

Thus, it will help to avoid redefinition warnings. By the way, what do you think about your ULONG_MAX which strongly conflicts with the standard definition?

If it carried copies of just the stuff it needs, you would not hit issues like this. I am pretty sure it would be only a small subset of the stuff that it ends up pulling in now.

Do you suggest me (and other Profiling API users) to copy-paste all interface definitions, MS types, and other required stuff from cor.h/corhdr.h/corprof.h/ntimage.h/etc headers to my project (due to conflicts with standard headers) instead of direct use of these files from upsource?

[janvorli]

Do you suggest me (and other Profiling API users) to copy-paste all interface definitions

Not at all. I meant that it would be better if such headers were provided for profiler developers.

what do you think about your ULONG_MAX which strongly conflicts with the standard definition?

This is the way it is defined in Windows headers. On Windows, long, unsigned long, LONG and ULONG types are 32 bit even on 64 bit platforms. CoreCLR depends on that. Even the profiler interfaces use LONG and ULONG that are 32 bit.

[sergiy-k]

It does not look like there is anything actionable here. Please re-activate the issue if you think otherwise or need help with anything. Thank you.

[k15tfu]

@sergiy-k We are stuck on me, sorry. I gonna make a PR for moving assert.h into cpp folder, as it was suggested above.

[k15tfu]

@janvorli Hi again! Sorry for long delay.

Here is PR with moved assert.h and few other platform specific files: dotnet/coreclr#23521

[k15tfu]

@janvorli and here is the PR for suppressing warnings for CHAR_BIT and other limits constants: dotnet/coreclr#23522