Skip to content

C: Support header files in ccall#1731

Merged
Smit-create merged 4 commits intolcompilers:mainfrom
Smit-create:i-1727
Apr 26, 2023
Merged

C: Support header files in ccall#1731
Smit-create merged 4 commits intolcompilers:mainfrom
Smit-create:i-1727

Conversation

@Smit-create
Copy link
Collaborator

@Smit-create Smit-create commented Apr 21, 2023

if (AST::is_a<AST::ConstantStr_t>(*call_d->m_keywords[i].m_value)) {
std::string header_name = AST::down_cast<AST::ConstantStr_t>(
call_d->m_keywords[i].m_value)->m_value;
bindc_name = s2c(al, header_name);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm using bindc_name to test the prototype and use it as a header file name which in LLVM is a function name to be called from C. Once this works, do we want to add a new argument in the function type to check which header file it is contained in?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we do. Probably not in FunctionType but in Function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think Function has a mechanism to store the c header file. It just has bindc_name for its interop with C.

| Function(symbol_table symtab, identifier name, ttype function_signature,
        identifier* dependencies, expr* args, stmt* body, expr? return_var,
        access access, bool deterministic, bool side_effect_free)

Should we go with this?

diff --git a/src/libasr/ASR.asdl b/src/libasr/ASR.asdl
index 34af5be3c..3f4b29b4a 100644
--- a/src/libasr/ASR.asdl
+++ b/src/libasr/ASR.asdl
@@ -363,7 +363,7 @@ ttype
     | CPtr()
     | TypeParameter(identifier param, dimension* dims)
     | FunctionType(ttype* arg_types, ttype? return_var_type,
-        abi abi, deftype deftype, string? bindc_name, bool elemental,
+        abi abi, deftype deftype, string? bindc_name, string? c_header, bool elemental,
         bool pure, bool module, bool inline, bool static, ttype* type_params,
         symbol* restrictions, bool is_restriction)

@certik
Copy link
Contributor

certik commented Apr 22, 2023

Thanks a lot @Smit-create for working on this! It looks like I found a workaround with #1732 and #1734 so that I am not blocked today anymore. This buys us time to design this well. We definitely need this feature.

| TypeParameter(identifier param, dimension* dims)
| FunctionType(ttype* arg_types, ttype? return_var_type,
abi abi, deftype deftype, string? bindc_name, bool elemental,
abi abi, deftype deftype, string? bindc_name, string? c_header, bool elemental,
Copy link
Contributor

Choose a reason for hiding this comment

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

c_header I think should go into Function, not be part of the type (neither logical nor physical: lfortran/lfortran#1601).

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

With those two changes this should be fine.

@Smit-create Smit-create marked this pull request as ready for review April 26, 2023 10:44
@Smit-create Smit-create requested a review from certik April 26, 2023 10:44
Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

There is one more thing that we should fix.

#include <math.h>
#include "complex.h"
#include "inttypes.h"
#include "math.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this change is a problem, it should use <, not ".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, thanks. Reverted that.

for (auto s: headers) {
to_include += "#include <" + s + ".h>\n";
for (auto &s: headers) {
to_include += "#include \"" + s + "\"\n";
Copy link
Contributor

Choose a reason for hiding this comment

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

For now let's use <, that is closer to correct.

We probably should distinguish between a header file that should be referenced with < (intrinsic ones) and " (user ones).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, thanks!

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

I think this is good enough now.

I noticed there are some default header files and macros always included and we should eventually handle them in the same way (put the macros into a header file and only include it if needed), but that's for later.

I think this is good to go in. Thanks for this @Smit-create, very important new feature.

@Smit-create Smit-create enabled auto-merge April 26, 2023 14:03
@Smit-create Smit-create merged commit a809eee into lcompilers:main Apr 26, 2023
@Smit-create Smit-create deleted the i-1727 branch April 26, 2023 14:42
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