search

KhronosGroup / OpenCL-Headers

Khronos OpenCL-Headers
Apache License 2.0
666 stars 236 forks source link

Add missing 'stdint.h' include to 'CL/cl.h' #164

Closed johnplate closed 3 years ago

johnplate commented 3 years ago

The header CL/cl.h is using intptr_t, but doesn't include the required stdint.h header.

On non-Windows platforms the header is already included by CL/cl_platform.h, but on Windows it causes a compilation error if stdint.h is not included before using CL/cl.h.

bashbaug commented 3 years ago

Hi @johnplate , thank you for continuing to submit PRs to fix issues like these!

Regarding this change:

  1. I'd like to improve our testing to be sure we catch similar issues like it in the future. I tried to do this here, by including the OpenCL headers before any other headers in our test files...

    https://github.com/bashbaug/OpenCL-Headers/tree/include-cl-header-first

    ...but I haven't been able to reproduce this issue. Am I getting lucky and is intptr_t is being defined by some other header? Is there something unique to your build configuration? (Note, this is baselined against a different branch with CI config updates, but this shouldn't affect the Windows testing.)

  2. Assuming that we do decide to explicitly include stdint.h, can you please move the include to cl_platform.h? It could either be included in the Windows-specific section (OK) or we can pull it out of any OS-specific sections (better) since we'll be including it for all OSes for now.

Thanks again!

johnplate commented 3 years ago

Hi @johnplate , thank you for continuing to submit PRs to fix issues like these!

My pleasure :)

...but I haven't been able to reproduce this issue. Am I getting lucky and is intptr_t is being defined by some other header? Is there something unique to your build configuration?

The CL headers are now in a project with really a lot of build configurations. Most were fine, but it broke our build on Windows with clang for arm64 target.

  1. Assuming that we do decide to explicitly include stdint.h, can you please move the include to cl_platform.h?

Sure, done. I have also documented the external dependency of the include position.

bashbaug commented 3 years ago

it broke our build on Windows with clang for arm64 target.

Ah, I see, I'm not sure that our Windows CI currently tests on arm64 so that's plausible.

Latest changes LGTM. Thanks!

johnplate commented 3 years ago

James is right, stdint.h was missing in Visual Studio 2005 and 2008. It would be great if the supported build configurations would be documented somewhere, so we could determine how to proceed.

bashbaug commented 3 years ago

Good point. I suppose we could check the MSVC version using _MSC_VER and only include stdint.h for versions of Visual Studio > 2010?

https://docs.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-160

jrprice commented 3 years ago

It would be great if the supported build configurations would be documented somewhere

Yep. We have issue #127 to track this, but we don't seem to have produced a conclusion yet.

jrprice commented 3 years ago

suppose we could check the MSVC version using _MSC_VER and only include stdint.h for versions of Visual Studio > 2010?

I'm not sure that's enough to get this building on the older MSVCs though. Where would they get their definition of intptr_t from? Do we need to introduce a cl_intptr and then either typedef __ptr cl_intptr or typedef intptr_t cl_intptr depending on the platform?

Presumably the headers don't build on MSVCs older than 2010 because they use intptr_t, and probably haven't for a while. If nobody has noticed, maybe that's a good indicator that we could set C99 as a minimum supported standard?

bashbaug commented 3 years ago

I think it's more that the headers from earlier versions of MSVC are picking up intptr_t from somewhere else, so they don't need to explicitly include stdint.h, which is good because it doesn't exist. 😄

In case you missed it, I haven't been able to reproduce this problem locally or via our CI, so we're probably just getting lucky on most of the Windows build configurations.

https://github.com/bashbaug/OpenCL-Headers/tree/include-cl-header-first

johnplate commented 3 years ago

Visual C++ provides intptr_t in stddef.h since forever, that's why Ben couldn't reproduce the bug. I have changed it now to only include stdint.h on Windows if clang is used or _MSC_VER >= 1600 (Visual Studio 2010)