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

Add and use const_ptr_api macro in openssl-sys #1359

Merged
merged 2 commits into from
Oct 25, 2020

Conversation

stbuehler
Copy link
Contributor

Alternative approach to #1348; actually tries to parse fn declarations and magically inserts *const or *mut.

@stbuehler
Copy link
Contributor Author

It seems I actually got it working. Any opinions regarding the syntax?

I think putting the cfg flags on the specific *const/*mut part is the right way to go; but it might work to use a little bit different syntax (e.g. *maybe_const($cfgflags)).

If you really want the cfg flags as attribute on the fn item, it probably would have to be the first attribute (otherwise it will take more recursions just to find it).

Also I think ctest will ignore any macro expansions failures (I had $fn_vis:vis in there, and it didn't check the item anymore, as the vis pattern was experimental back then; not sure about hitting the recursion limit); which is dangerous, because it simply won't generate tests for those items - but they are still present.

}
const_ptr_api! {
extern "C" {
pub fn ASN1_STRING_to_UTF8(out: *mut *mut c_uchar, s: #[const_ptr_if(any(ossl110, libressl280))] ASN1_STRING) -> c_int;
Copy link
Owner

Choose a reason for hiding this comment

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

Can the macro be changed to only apply to the individual extern definition rather than wrapping the whole extern "C" block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Syntactically speaking I think the macro can do that, but rustc doesn't like it; playground example fails with error[E0428]: the name ASN1_STRING_to_UTF8 is defined multiple times; but when looking at the generated code through Tools -> Expand macros it seems to be doing the right thing.

I'm pretty sure that is the reason why the cfg_if! blocks have internal extern "C" {...} blocks too; probably the same limitation. And even if rustc got fixed and you raise the MSRV, ctest will still fail, as it uses an old fork if a rustc internal crates to do parsing and macro expansion.

Passing extern "C" { ... } can be removed of course, but it can't be part of another extern "C" { ... } block and needs to emit extern "C" { ... } for the fn items.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah bummer. This seems good then!

@sfackler
Copy link
Owner

Nice! Yeah, the cfg syntax in your macro seems fine.

Just to make sure I understand - ctest does still include tests for the macro-defined functions, right? It just doesn't fail to compile if the macro is misused but the normal openssl-sys build would fail at that point anyway?

@stbuehler
Copy link
Contributor Author

Nice! Yeah, the cfg syntax in your macro seems fine.

Just to make sure I understand - ctest does still include tests for the macro-defined functions, right? It just doesn't fail to compile if the macro is misused but the normal openssl-sys build would fail at that point anyway?

After I removed the $fn_vis:vis handling and just hardcoded it as pub it did, yes. I think if the macro expansion fails it pretends the macro expanded to an empty output.

As ctest uses an (unmaintained) old fork of a rustc internal crate the current rustc won't necessarily fail (also it seems to use a hardcoded recursion limit of 128).

@stbuehler stbuehler force-pushed the const-ptr-api-macro branch 2 times, most recently from 3bec303 to 2c42cef Compare October 25, 2020 11:24
so it also compiles on rust 1.31; also same limit as used by
systest/ctest.
@stbuehler stbuehler force-pushed the const-ptr-api-macro branch from 2c42cef to 1dbeb8c Compare October 25, 2020 11:46
@stbuehler stbuehler marked this pull request as ready for review October 25, 2020 13:17
@stbuehler
Copy link
Contributor Author

macos seems to be down in circleci...

I also opened gnzlbg/ctest#101 - if this gets merged and released, I think systest could be made more reliable to check all items (i.e. fail if macro expansion fails).

@sfackler sfackler merged commit f958176 into sfackler:master Oct 25, 2020
@stbuehler stbuehler deleted the const-ptr-api-macro branch October 25, 2020 16:48
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.

None yet

2 participants