Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WAR for __has_include #124

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

WAR for __has_include #124

wants to merge 6 commits into from

Conversation

maddyscientist
Copy link
Collaborator

This WAR addresses an issue where nvrtc can fail to correctly deal with statements of the form

#if __has_include(<HEADER_NAME>)

where even if we have the header present in the set of includes it fails to return true. The WAR here is to search for such statements when we load up headers, and if the header in question in manually found, we replace the statement with

#if 1

and if not, replace it with

#if 0

With this WAR in place, we no longer need the unneeded includes in the builtin_numeric_cuda_std_limits_program program code, since these will be automatically included now as per @robertmaynard's original intention.

jitify.hpp Outdated Show resolved Hide resolved
jitify.hpp Outdated Show resolved Hide resolved
jitify.hpp Outdated Show resolved Hide resolved
jitify.hpp Outdated
<< filename << ":" << linenum << std::endl;
#endif
// Try loading from filesystem
bool found_file = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not important, but I wonder if this would be cleaner as an immediately-called lambda (bool found_file = [&] { ... }(); so that found_file = true can be replaced with return true etc.).

@maddyscientist
Copy link
Collaborator Author

I think I've addressed all the comments, barring the final lambda question. Not sure this is necessarily cleaner.

jitify.hpp Outdated
Comment on lines 709 to 712
if (has_name.find("\"", header_start) == std::string::npos)
throw std::runtime_error("Malformed __has_include statement (" +
filename + ":" + std::to_string(linenum) +
")");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be unified between both <> and "" paths by setting a header_end instead of header_count and checking if header_end == std::string::npos after the if/elseif. (It should also probably throw if neither < nor " is found?).

jitify.hpp Outdated
Comment on lines 753 to 759
if (found_file) {
line = cleanline.substr(0, has_include_start) + "(1)" +
cleanline.substr(close + 1);
} else {
line = cleanline.substr(0, has_include_start) + "(0)" +
cleanline.substr(close + 1);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A ternary would avoid the code duplication here:
line = cleanline.substr(0, has_include_start) + (found_file ? "(1)" : "(0)") + cleanline.substr(close + 1);

@leofang
Copy link
Member

leofang commented Sep 8, 2023

Was trying this PR and saw this

cupy.cuda.compiler.JitifyException: Malformed __has_include statement (libcxx/include/__config:249)

The line number aside (which might be shifted for some reason), I see libcudacxx has this macro
https://github.com/NVIDIA/libcudacxx/blob/a57dbed580e49b14ac1ad7f98496176407208aa4/include/cuda/std/detail/libcxx/include/__config#L212-L214
Could it be causing the error?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants