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

Improve macro defined constants to recognize #define CONSTANT ((int) 1) #316

Open
azerupi opened this issue Dec 4, 2016 · 32 comments
Open
Labels
A-macros enhancement help wanted rust-for-linux Issues relevant to the Rust for Linux project

Comments

@azerupi
Copy link

azerupi commented Dec 4, 2016

In the header files I am trying to make bindings for, some constants are defined like this:

#define kOfxStatFailed  ((int)1)
#define kOfxStatErrFatal ((int)2)
// ...

Can this pattern be supported? Maybe I can attempt to add this if I get some guidance :)

@emilio
Copy link
Contributor

emilio commented Dec 5, 2016

This can certainly be supported, though it'd be kind of a one-off.

Indeed I think this eventually worked (by chance) because at one point we tried to find just the first integer literal token.

I think the best approach for this is something like:

  • Stripping parentheses of the cexpr_tokens variable in src/ir/var.rs.
  • Try to split a leading cast on those tokens (something like: First one is punctuation and the character b'(', next token is a literal and a well known type (let's support only int to begin with, for example), and then another punctuation with the b')' char.
  • With that stripped, the rest of the code should just work. Also, we can detect what the cast is, and save it to generate the correct type, that'd be a plus :)

Do you want to try implementing it? I'm not sure I'll find the time myself in the near term. If you want, happy to elaborate more if needed :)

@azerupi
Copy link
Author

azerupi commented Dec 5, 2016

Yes, I will give it a try :)
I'll let you know if I get stuck.

@nox
Copy link
Contributor

nox commented Dec 5, 2016

Can't we just make cexpr support casts?

@azerupi
Copy link
Author

azerupi commented Dec 5, 2016

I think it would be better too. The code I wrote felt out of place and it would be better to handle all macro const stuff in the same place.

@emilio
Copy link
Contributor

emilio commented Dec 6, 2016

Yeah, that'd work better. I've also just realized we can't just strip wrapping parentheses since they can change the precedence of operators.

@bbigras
Copy link

bbigras commented Jan 19, 2017

I have a similar case. Anyone knows how I can use the following in Rust in the meantime?

#define GSS_C_NO_NAME ((gss_name_t) 0)
#define GSS_C_NO_BUFFER ((gss_buffer_t) 0)

@emilio
Copy link
Contributor

emilio commented Jan 19, 2017

I guess you'd need to define them manually :/

@fitzgen
Copy link
Member

fitzgen commented Jan 19, 2017

@BrunoQC if gss_foo_t are typedefs of pointers, then this should work:

0 as *mut _

If they are typedefs of some kind of integer type, then this should work:

0 as _

Otherwise, you'll need to use std::mem::transmute (maybe with lazy_static if you want it to be a global):

use std::mem;

let no_name: gss_name_t = unsafe { mem::transmute(0) };

@target-san
Copy link

I have a rather stupid idea. Which can 'shoot' tho.
For each macro definition, construct a stub C++ file

#include "source_header.h"
const auto CONSTANT = MACRO;

Then force CLang to process it and give type and value of CONSTANT. If it doesn't compile, then it's not a macro constant.
It may be rather slow - but I'd presonally wait once to have proper constants.

@ristew
Copy link

ristew commented Mar 26, 2018

I imagine the following could never work with this
#define ECL_T ((cl_object)(cl_symbols+1))
where cl_symbols is an array holding structs.

@jethrogb
Copy link
Contributor

Bindgen team: tag this with help wanted?

@ristew We currently don't support handling anything but literals, not including struct initializers. Never say never, but I wouldn't expect support for things like that any time soon.

@xxks-kkk
Copy link

xxks-kkk commented Sep 1, 2018

Currently, I have the following macro in C, which is completely get ignored by the bindgen, is there anyway to create it automatically?

#define SPDK_NOTICELOG(...) \
	spdk_log(SPDK_LOG_NOTICE, __FILE__, __LINE__, __func__, __VA_ARGS__)
#define SPDK_WARNLOG(...) \
	spdk_log(SPDK_LOG_WARN, __FILE__, __LINE__, __func__, __VA_ARGS__)
#define SPDK_ERRLOG(...) \
	spdk_log(SPDK_LOG_ERROR, __FILE__, __LINE__, __func__, __VA_ARGS__)

@jethrogb
Copy link
Contributor

jethrogb commented Sep 1, 2018

Supporting #define function calls, especially with printf-style formatting, is probably not happening anytime soon.

@xxks-kkk
Copy link

xxks-kkk commented Sep 3, 2018

@jethrogb Thanks for the reply. I'm wondering if that's something I can do manually in Rust? If so, I can write them on my own but I can't find some proper example on how to do so.

Thanks much!

@emilio
Copy link
Contributor

emilio commented Sep 3, 2018

@xxks-kkk I'd consider using the log crate for your rust code. You can then add a logger implementation that forwards to spdk_log if you need to, but it'd probably be unnecessary.

@Lokathor
Copy link
Contributor

I just hit this issue myself:

/* Used as the device ID for mouse events simulated with touch input */
#define SDL_TOUCH_MOUSEID ((Uint32)-1)

@elichai
Copy link
Contributor

elichai commented Nov 8, 2019

any news here? would also be nice to specify what types I want for those defines, a lot of time it's i32 and not u32

@Lokathor
Copy link
Contributor

Lokathor commented Nov 8, 2019

Perhaps an option to have define values be emitted on the rust side as macros that spit out literals, then the literals can fit into any type

#define foo 1

becomes

macro_rules! foo { () = { 1 } }

@elichai
Copy link
Contributor

elichai commented Nov 8, 2019

On one hand that's somewhat more correct (Altough Rust's literal rules don't match C's)
on the other that will bloat everything up.

@elichai
Copy link
Contributor

elichai commented Nov 8, 2019

And might ruin compile time?

@Lokathor
Copy link
Contributor

Lokathor commented Nov 8, 2019

I can't imagine it would ruin compile time. It's the same cost as a single token inline function basically.

But we also don't need it always in macro form, just for specific designated elements that need to be both signed and unsigned.

@emilio
Copy link
Contributor

emilio commented Nov 9, 2019

The right way to specify a type is using static consts, fwiw: static const int32_t FOO = 1;.

There's a way to specify the type for the #define:

fn int_macro(&self, _name: &str, _value: i64) -> Option<IntKind> {

@elichai
Copy link
Contributor

elichai commented Nov 9, 2019

The right way to specify a type is using static consts, fwiw: static const int32_t FOO = 1;.

There's a way to specify the type for the #define:

fn int_macro(&self, _name: &str, _value: i64) -> Option<IntKind> {

I looked into the code now and the logic seem right but it looks like it doesn't work properly.
the only place it's called is: https://github.com/rust-lang/rust-bindgen/blob/master/src/ir/var.rs#L210 which then calls https://github.com/rust-lang/rust-bindgen/blob/master/src/ir/var.rs#L117

which seem like it will choose i32 if less then max or u32 if more but in practice that's not what happens:

test.h:

#define AAA 5

bindgen test.h output:

/* automatically generated by rust-bindgen */

pub const AAA: u32 = 5;

As you can see it chooses u32 even though it should be i32

EDIT: I'm wrong, the logic there is flawed. it chooses i32 only if it's a negative number.

@elichai
Copy link
Contributor

elichai commented Nov 9, 2019

Ok.
While trying to come up with a function that will match more closely the C11 standard (6.4.4.1p5.) I found the following problem:

#define AAA 9223372036854775808ULL

bindgen returns:

pub const AAA: i64 = -9223372036854775808;

The problem seems to be because of the use of Wrapping(i64), I think the better way would be to use i128 and then have 2 algorithms, one for if it has the u postfix another if it doesn't (not sure how that's represented in clang's AST).
Anyway this seems out of my depth currently :/ @emilio

@emilio
Copy link
Contributor

emilio commented Nov 9, 2019

That needs cexpr support, basically: https://github.com/jethrogb/rust-cexpr is what we use to parse the macros and only has i64 support. It should probably at the very least return a u64 for that case.

@elichai
Copy link
Contributor

elichai commented Nov 9, 2019

Related:
#1594
jethrogb/rust-cexpr#16

@jethrogb
Copy link
Contributor

There's an open PR that will improve things somewhat jethrogb/rust-cexpr#15

bunnie added a commit to betrusted-io/betrusted-ec that referenced this issue Feb 20, 2020
bindgen won't create CONST for #define macros that are complicated.
fix is to fork the file, manual edit, generate.
molpopgen added a commit to tskit-dev/tskit-rust that referenced this issue Dec 9, 2020
@SeleDreams
Copy link

I've been having this issue while implementing nintendo DS support to rust. Some registers defined in the libnds SDK aren't getting defined in rust

/*!	\brief Key input register.
	On the ARM9, the hinge "button", the touch status, and the
	X and Y buttons cannot be accessed directly.
*/
#define	REG_KEYINPUT	(*(vuint16*)0x04000130)

this is one of them that does not get exposed to rust

@Lokathor
Copy link
Contributor

Speaking from my experience with the GBA, it's unlikely that you'd want to use C headers for defining your MMIO anyway. Since C doesn't have repr(transparent), they also miss out on having any good level of typing on their MMIO.

@SeleDreams
Copy link

Speaking from my experience with the GBA, it's unlikely that you'd want to use C headers for defining your MMIO anyway. Since C doesn't have repr(transparent), they also miss out on having any good level of typing on their MMIO.

could you elaborate ?
at the moment I've been using stuff like
pub const REG_KEYINPUT : *const u16 = 0x04000130 as *const u16;
in order to represent these defines. I then read and write them through read_volatile and write_volatile
is it a bad way to do it ?

to be honest I've more programmed in C++ before, I'm in a phase where I transition to rust.
atm I'm doing a kind of 1 to 1 wrapper to the libnds sdk and I planned to make a safe wrapper after once i got the unsafe wrapper working. like they do for the 3ds with libctru-rs

@Lokathor
Copy link
Contributor

Well, you can have a look at the gba crate, https://github.com/rust-console/gba, particularly the MMIO module should be very familiar to you since the DS is largely an extension of the GBA.

I don't want to fill up this issue with too much stuff that's not about bindgen, so just open an issue or discussion in the gba repo if you have more questions after having a look. It certainly took me a few iterations to settle on a (mostly) satisfying design so don't worry if you go through a few drafts.

@LunarLambda
Copy link

LunarLambda commented Feb 17, 2023

Ah, funny, I seemed to remember that bindgen actually -did- support basic casts, but alas:

https://github.com/LunarLambda/sdk-seven/blob/65cd3372fb0fac7c7d1d8529498097d8c3ddfb9f/libseven/include/seven/base.h#L36-L52

@emilio emilio added the rust-for-linux Issues relevant to the Rust for Linux project label Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macros enhancement help wanted rust-for-linux Issues relevant to the Rust for Linux project
Projects
None yet
Development

No branches or pull requests