Page MenuHomePhabricator

[OpenMP] Remove 'stdbool.h' from OpenMP header wrappers
AbandonedPublic

Authored by jhuber6 on Aug 10 2022, 7:08 PM.

Details

Summary

This patch removes the stdbool.h include path from the clang OpenMP
header wrappers. These are used to predeclare the device-side functions
for common headers. The bool header should not be necessary and causes
some problems with redeclarations.

Fixes #57066

Diff Detail

Event Timeline

jhuber6 created this revision.Aug 10 2022, 7:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2022, 7:08 PM
jhuber6 requested review of this revision.Aug 10 2022, 7:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2022, 7:08 PM

bool is used by a function declaration __ockl_fdot2 in clang/lib/Headers/__clang_hip_libdevice_declares.h, which is included by clang/lib/Headers/openmp_wrappers/__clang_openmp_device_functions.h.

jhuber6 updated this revision to Diff 451875.Aug 11 2022, 8:57 AM

Changing bool usage to use _Bool if compiling for C so we don't need to include the extra header for this single declaration.

I'm fine with the change. @JonChesterfield WDYT?

JonChesterfield added a comment.EditedAug 11 2022, 11:10 PM

We should write down (maybe in the commit message) what this is fixing. The linked bug has someone defining bool as a workaround for msvc which shouldn't be applied when not compiling with msvc, so superficially it looks like this patch works around broken application code. That doesn't seem a good thing.

I don't think this will compile as c89 (not that it would with the bool header either). I don't know what fdot2 does, shall we just cut out the whole function declaration under openmp?

This looks good to me, and I agree we should document what this is fixing. Any update on if/when this will land?

In my opinion, there's nothing broken about the user code (definitely contrived, though). They didn't ask for stdbool.h so there should not be a redeclaration error. This doesn't seem to be an issue when compiling for NVIDIA or host-only OpenMP, either.

jhuber6 added a comment.EditedAug 25 2022, 9:46 AM

This looks good to me, and I agree we should document what this is fixing. Any update on if/when this will land?

In my opinion, there's nothing broken about the user code (definitely contrived, though). They didn't ask for stdbool.h so there should not be a redeclaration error. This doesn't seem to be an issue when compiling for NVIDIA or host-only OpenMP, either.

The example code in the bug report suggests the user is defining their own boolean type. I think the C and C++ standard is pretty clear that __ and _[A-Z] identifiers are reserved and may clash at any point if defined by the user. This is typically why these are provided via headers that are guarded appropriately as stdbool.h is. I'm also not sure if this is a problem upstream, or it just reflects some vendor's implementation of it. Maybe others want to comment on this.

The user didn't define any __ or _[A-Z] identifiers, though?

The user didn't define any __ or _[A-Z] identifiers, though?

Am I misunderstanding the test input?

/* Visual Studio < 2013 does not have stdbool.h so here it is a replacement: */
#if defined __STDC__ && defined __STDC_VERSION__ && __STDC_VERSION__ >= 199901L
/* have a C99 compiler */
typedef _Bool bool;
#else
/* do not have a C99 compiler */
typedef unsigned char bool;
#endif

I assumed this was the user defining this on their own.

They are defining their own bool, which aliases to the built-in _Bool (which is reserved, as you noted with _[A-Z]). I thought bool was fair game unless they included stdbool.h?

They are defining their own bool, which aliases to the built-in _Bool (which is reserved, as you noted with _[A-Z]). I thought bool was fair game unless they included stdbool.h?

You're right, I was reading it wrong. It should be "legal" to typedef something if no one else has done it.

I'm still wondering if this is really our fault as it's still likely to conflict. The <stdbool.h> header supplies __bool_true_false_are_defined exactly for this reason. Can the user not check this locally to prevent re-declaration? I think it's perfectly reasonable to include system files as part of a toolchain.

I think it's perfectly reasonable to include system files as part of a toolchain.

I think it comes down to a matter of inconveniencing the user versus the developer. We usually go with the latter. I don't particularly agree with your statement I quoted above, but I can't say I'm an expert in this area. I'll see what others have to say. I've already pulled this into our fork so it doesn't make a difference to us, but I did think it would be useful to get input from the community on whether or not we should pull this in to upstream.

Thanks for your input and responses.

jdoerfert requested changes to this revision.Aug 29 2022, 9:37 AM

I think the code as is upstream is fine. The test input is problematic. There is no guarantee, or even any argument, that stdbool is not included by the compiler or any header (system or not). If the user writes conflicting cod with the system, that calls for problems down the line.

Long story short, I'd abandon this for now.

This revision now requires changes to proceed.Aug 29 2022, 9:37 AM
jhuber6 abandoned this revision.Aug 29 2022, 9:45 AM

I think the code as is upstream is fine. The test input is problematic. There is no guarantee, or even any argument, that stdbool is not included by the compiler or any header (system or not). If the user writes conflicting cod with the system, that calls for problems down the line.

Long story short, I'd abandon this for now.

I agree, I will abandon this revision and close the associated bug.

I think it's perfectly reasonable to include system files as part of a toolchain.

I think it comes down to a matter of inconveniencing the user versus the developer. We usually go with the latter. I don't particularly agree with your statement I quoted above, but I can't say I'm an expert in this area. I'll see what others have to say. I've already pulled this into our fork so it doesn't make a difference to us, but I did think it would be useful to get input from the community on whether or not we should pull this in to upstream.

Thanks for your input and responses.

I'm glad this helped you fix it on your end, thanks for opening an issue for this.