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

[WIP] Initial bitfields support #215

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@XVilka
Copy link
Contributor

XVilka commented Jan 3, 2019

Trying to address the problem of supporting bitfields (#205), please see the initial draft and provide your opinion/criticism/suggestions.

Please note, for now I implemented bitfields only for an assumption that it is GCC-like behavior, with __attribute__((packed)) or #pragma pack (push,1)

At this point only reading is implemented. Once it is polished and ready - I will do writing too, since more complex.

@XVilka XVilka changed the title Initial bitfields support [WIP] Initial bitfields support Jan 3, 2019

@XVilka

This comment has been minimized.

Copy link
Contributor

XVilka commented Jan 3, 2019

Travis error seems unrelated:

opam show /tmp/opam-ci-192 --raw
[WARNING] Failed checks on cstruct package definition from source at file:///tmp/opam-ci-192:
             error 57: Synopsis and description must not be both empty
Errors.
             error 57: Synopsis and description must not be both empty
'unset TESTS; OPAMYES=1 export OPAMYES; set -uex; cp cstruct.opam /tmp/opam-ci-192 && opam show /tmp/opam-ci-192 --raw | opam lint -' exited 1. Terminating with 1
The command "bash -ex ./.travis-docker.sh" exited with 1.

@XVilka XVilka force-pushed the XVilka:bitfields branch 6 times, most recently from 0aee377 to 4721507 Jan 3, 2019

@XVilka

This comment has been minimized.

Copy link
Contributor

XVilka commented Jan 4, 2019

Please provide your feedback, so I can change the PR accordingly.

Show resolved Hide resolved ppx/ppx_cstruct.ml Outdated
@avsm

This comment has been minimized.

Copy link
Member

avsm commented Jan 4, 2019

Thanks very much for all the hard work you've put into this! I'm a little torn about the approach, since we are reimplementing some difficult pieces of a C compiler directly within OCaml. In particular, the restriction on attributed-packed is what has me worried.

Before I start reviewing this, I wanted to open one alternate possibility. The ctypes library has comprehensive infrastructure to do compile-time probing of the C compiler in use (including cross-compilation) in order to determine various struct layouts. I've wanted a cstruct->ctypes bridge for some time, so one possibility is to use Ctypes' precise definitions of C layout to implement the Cstruct ppx for bitfields.

Another high level question: what is the usecase you want to solve with bitfields? I just want to make sure that the restrictions placed on the current PR (e.g. packed structs only) are compatible with what you need (e.g. is it a bitfield from a network protocol?)

@XVilka

This comment has been minimized.

Copy link
Contributor

XVilka commented Jan 5, 2019

@avsm yes, it is required for file formats parsing. Problem, there is no exact definition of even C compiler behavior in case of bitfields (this is why modern formats and protocols prefer to avoid them). Some compilers behave one way, some - another way. Most of the formats and protocols rely on the packed tightly, sequential bitfield layout though.

@XVilka XVilka force-pushed the XVilka:bitfields branch 4 times, most recently from 9575208 to 4342afe Jan 7, 2019

@XVilka

This comment has been minimized.

Copy link
Contributor

XVilka commented Jan 7, 2019

I cleaned up the code, solved the problem with length handling and now tests are green.

Strange... seems running dune runtest ppx_test doesn't really fail if I put a wrong test, or even doesn't print anything if I add Printf.printf "blabla\n";

@XVilka XVilka force-pushed the XVilka:bitfields branch from 4342afe to d83cb33 Jan 7, 2019

Show resolved Hide resolved ppx/ppx_cstruct.ml Outdated
@XVilka

This comment has been minimized.

Copy link
Contributor

XVilka commented Jan 7, 2019

Added also a fix for hexdump, but don't know how to embed the function properly in the PPX.

@XVilka XVilka force-pushed the XVilka:bitfields branch from a134516 to 07f64fa Jan 7, 2019

@XVilka

This comment has been minimized.

Copy link
Contributor

XVilka commented Jan 7, 2019

Nevermind, fixed also, now it is ready for the review - if the approach is good, I will add also the setters code.

@XVilka

This comment has been minimized.

Copy link
Contributor

XVilka commented Jan 7, 2019

Wait, it will not work for big endian machines, since on BE it should read bit-by-bit from the most significant bit, not just swap bytes and read from the least significant bits.

@XVilka

This comment has been minimized.

Copy link
Contributor

XVilka commented Jan 8, 2019

Ok, pushed also the big endian proper support, tested with various bitfield layouts.

@XVilka XVilka force-pushed the XVilka:bitfields branch 2 times, most recently from 7878721 to 8de8421 Jan 8, 2019

@XVilka

This comment has been minimized.

Copy link
Contributor

XVilka commented Jan 9, 2019

With big-endian bitfields I met one problem - basically I need to know the size of the structure to count bits from the end, but t.len stores the size of the Cstruct buffer instead. Any advice?

@XVilka XVilka force-pushed the XVilka:bitfields branch from 8de8421 to e43e34f Jan 9, 2019

@XVilka

This comment has been minimized.

Copy link
Contributor

XVilka commented Jan 15, 2019

So? What do you think about this approach?

@XVilka

This comment has been minimized.

Copy link
Contributor

XVilka commented Jan 21, 2019

Ping?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment