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

Use of _IO, _IOR, etc. #141

Closed
russel opened this issue Jun 3, 2017 · 22 comments
Closed

Use of _IO, _IOR, etc. #141

russel opened this issue Jun 3, 2017 · 22 comments
Labels

Comments

@russel
Copy link
Contributor

russel commented Jun 3, 2017

The libdvbv5 headers connect with the Linux DVB API, which means use of IOCTLs. The standard way of doing this for Linux is with a set of macros, including _IO, _IOR, which are defined in various ioctl.h files. Is it worth handling these within DStep or should it be seen as a "done manually" thing made available via another route?

@russel
Copy link
Contributor Author

russel commented Jun 3, 2017

I manually converted /usr/include/asm-generic/ioctl.h but this turns:

#define _IOC(dir,type,nr,size) \
	(((dir)  << _IOC_DIRSHIFT) | \
	 ((type) << _IOC_TYPESHIFT) | \
	 ((nr)   << _IOC_NRSHIFT) | \
	 ((size) << _IOC_SIZESHIFT))

#define _IOC_TYPECHECK(t) (sizeof(t))

/* used to create numbers */
#define _IO(type,nr)		_IOC(_IOC_NONE,(type),(nr),0)
#define _IOR(type,nr,size)	_IOC(_IOC_READ,(type),(nr),(_IOC_TYPECHECK(size)))

into

extern (D) auto _IOC(T0, T1, T2, T3)(auto ref T0 dir, auto ref T1 type, auto ref T2 nr, auto ref T3 size)
{
    return (dir << _IOC_DIRSHIFT) | (type << _IOC_TYPESHIFT) | (nr << _IOC_NRSHIFT) | (size << _IOC_SIZESHIFT);
}

extern (D) size_t _IOC_TYPECHECK(T)(auto ref T t)
{
    return t.sizeof;
}

/* used to create numbers */
extern (D) auto _IO(T0, T1)(auto ref T0 type, auto ref T1 nr)
{
    return _IOC(_IOC_NONE, type, nr, 0);
}

extern (D) auto _IOR(T0, T1, T2)(auto ref T0 type, auto ref T1 nr, auto ref T2 size)
{
    return _IOC(_IOC_READ, type, nr, (_IOC_TYPECHECK(size)));
}

_IO is fine, however _IOR is a real problem. For example:

enum FE_READ_STATUS = _IOR('o', 69, fe_status_t);
enum FE_READ_BER = _IOR('o', 70, __u32);
enum FE_READ_SIGNAL_STRENGTH = _IOR('o', 71, __u16);
enum FE_READ_SNR = _IOR('o', 72, __u16);
enum FE_READ_UNCORRECTED_BLOCKS = _IOR('o', 73, __u32);

it is not posible to pass the type names as function parameters in D where they were fine passed to macros in C and C++.

@jacob-carlborg
Copy link
Owner

The libdvbv5 headers connect with the Linux DVB API, which means use of IOCTLs. The standard way of doing this for Linux is with a set of macros, including _IO, _IOR, which are defined in various ioctl.h files. Is it worth handling these within DStep or should it be seen as a "done manually" thing made available via another route?

These files need to be separately converted, either using DStep or manually.

@jacob-carlborg
Copy link
Owner

it is not posible to pass the type names as function parameters in D where they were fine passed to macros in C and C++.

Yeah, that's again a problem with macros. When the compiler is analyzing a macro, it doesn't know what a parameter will be, a type, a value, a variable or something else. Not sure if that's solvable.

BTW, is that macro (_IOC) using a shift operator on a type?

@russel
Copy link
Contributor Author

russel commented Jun 3, 2017

No the shift is on T.sizeof which hopefully is an integer.

@jacob-carlborg
Copy link
Owner

Aha, you pass a type to what is called size and a character to what is called type. That's, that's not confusing at all 😃.

@russel
Copy link
Contributor Author

russel commented Jun 3, 2017

These Linux kernel folk will have their little jokes.

Though it does end up being a size, via an intermediate macro call. I haven't sussed out the use of character 'o' yet.

@jacob-carlborg
Copy link
Owner

As I mentioned in the newsgroup, I think replacing all macro parameters with a variadic template parameter will work. I manually adjusted the above D code to:

enum _IOC_READ = 2;
enum _IOC_DIRSHIFT = 1;
enum _IOC_TYPESHIFT = 1;
enum _IOC_NRSHIFT = 1;
enum _IOC_SIZESHIFT = 1;

extern (D) auto _IOC(Args... /* dir, type, nr, size */)() if (Args.length == 4)
{
    return (Args[0] << _IOC_DIRSHIFT) | (Args[1] << _IOC_TYPESHIFT) | (Args[2] << _IOC_NRSHIFT) | (Args[3] << _IOC_SIZESHIFT);
}

extern (D) size_t _IOC_TYPECHECK(t)()
{
    return t.sizeof;
}

/* used to create numbers */
extern (D) auto _IO(Args... /* type, nr */)() if (Args.length == 2)
{
    return _IOC!(_IOC_NONE, Args[0], Args[1], 0);
}

extern (D) auto _IOR(Args... /* type, nr, size */)() if (Args.length == 3)
{
    return _IOC!(_IOC_READ, Args[0], Args[1], (_IOC_TYPECHECK!(Args[2])));
}

enum FE_READ_STATUS = _IOR!('o', 69, int);
enum FE_READ_BER = _IOR!('o', 70, int);
enum FE_READ_SIGNAL_STRENGTH = _IOR!('o', 71, int);
enum FE_READ_SNR = _IOR!('o', 72, int);
enum FE_READ_UNCORRECTED_BLOCKS = _IOR!('o', 73, int);

I don't know if it works but at least it compiles 😃.

@russel
Copy link
Contributor Author

russel commented Jun 3, 2017

That is quite neat. I think though the best bet is to just do .sizeof in the original call – and dispense with the _IOC_TYPECHECK.

I will have to create a workflow where I have the current working set up and then a way of melding changes if libdvbv5 changes. So continuously use dstep but only to update a manually amended version of the header files as needed.

[Having now read the thread.]

I might just go with the variadic template with constraint you suggest. It seems idiomatic Phobos technique.

[Having now tried both.]

I think I prefer the function _IO, _IOR, rather than Args... template. I think I am going to choose to manually construct this with proper types rather than template types, and deal explicitly with the 'o'.

@russel
Copy link
Contributor Author

russel commented Jun 4, 2017

I think this issue is a very specific one, and there are two workarounds via manual intervention, so I am going to close this.

@russel russel closed this as completed Jun 4, 2017
@jacob-carlborg
Copy link
Owner

Since it's possible to pass types to macros, I'm considering this a bug in DStep which should be fixed.

@jacob-carlborg
Copy link
Owner

@russel BTW, DStep has a flag, --skip, that allows you to skip translating some specific symbols. This might be of help here, if you want to translate these macros by hand.

@ciechowoj
Copy link
Contributor

ciechowoj commented Jul 9, 2017

What do you think about embedding some info into dstep about widely used headers so that it can make educated guess how to translate specific functions?

The problem with approach:

enum FE_READ_SIGNAL_STRENGTH = _IOR!('o', 71, int);

is then this macro couldn't be used in case when its argument is a variable, whose value is unknown at the compile time.

We can infer the correct translation from usage, but e.g. in ioctl.h there are no single usage. It's even hard to guess for human if the argument should be type or variable...

By the info I consider some structured file (json) with entries like:

{
    "macrodefinition": "_IOR(type,nr,size)",
    "includefileregex": "*/ioctl.h",
    "signature": "_IOR(int, int, <type>)"
}

or

{
    "hint_type": "example_usage",
    "example": "_IOR('o', 74, int)"
}

The user would be allowed to override it or provide its own rules.

@jacob-carlborg
Copy link
Owner

Hmm, I'll have to think about this. My initial reaction is that is sounds complicated.

@jacob-carlborg
Copy link
Owner

@ciechowoj do you think it would be possible to figure out the signature by looking at the body of the macro?

@ciechowoj
Copy link
Contributor

ciechowoj commented Jul 12, 2017 via email

@jacob-carlborg
Copy link
Owner

Of course 😞.

@ciechowoj
Copy link
Contributor

ciechowoj commented Sep 30, 2017

Hi guys. I've created an experimental implementation which may solve the problem:
#173
Unit test:
https://github.com/jacob-carlborg/dstep/pull/173/files#diff-2e084913cee59a6ba95bde92534a2b4a

It is probably what Russel suggested to make .sizeof in the topmost call. The implementation tries to hoist the calls to sizeof to the top-most level possible and replaces the indirect types with sizeof (see the example in the unit test).

What do you think about this, I would propose to make it an optional feature of dstep which could be enabled from the command line.

@jacob-carlborg
Copy link
Owner

If it's intended to use _IOR it changes the API. Otherwise I guess it's fine.

ciechowoj added a commit to ciechowoj/dstep that referenced this issue Oct 19, 2017
ciechowoj added a commit to ciechowoj/dstep that referenced this issue Oct 19, 2017
ciechowoj added a commit to ciechowoj/dstep that referenced this issue Oct 19, 2017
@jacob-carlborg
Copy link
Owner

@russel what do you think of the translation in a PR ciechowoj just created: https://github.com/jacob-carlborg/dstep/pull/178/files#diff-2e084913cee59a6ba95bde92534a2b4a?

@russel
Copy link
Contributor Author

russel commented Oct 20, 2017

@jacob-carlborg I will not be able to look at this immediately I'm afraid, stuff to do with PyConUK and then a short break, and other stuff. I should be able to delve deeply after that. Sorry for this.

@ciechowoj
Copy link
Contributor

ciechowoj commented Oct 20, 2017 via email

@jacob-carlborg
Copy link
Owner

Anyway, can we merge the PR without closing the bug?

I need to review it first, at least 😉.

ciechowoj added a commit to ciechowoj/dstep that referenced this issue Oct 21, 2017
ciechowoj added a commit to ciechowoj/dstep that referenced this issue Oct 21, 2017
ciechowoj added a commit to ciechowoj/dstep that referenced this issue Mar 16, 2018
jacob-carlborg added a commit that referenced this issue Mar 18, 2018
…-usage

Fix #141: Improve type inference for macros, in particular handle type arguments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants