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.

Sign up for GitHub

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

Jump to bottom

[Discussion] FFI - export os.sizeof and os.alignof mappings #1759

Closed
wants to merge 7 commits into from
Closed

[Discussion] FFI - export os.sizeof and os.alignof mappings #1759

wants to merge 7 commits into from

Conversation

TooTallNate
Copy link
Contributor

Related to #1750.

This PR adds os.sizeof and os.alignof Object mappings. For example, on a 64-bit OS:

> os.sizeof
{ int8: 1,
  uint8: 1,
  int16: 2,
  uint16: 2,
  int32: 4,
  uint32: 4,
  int64: 8,
  uint64: 8,
  float: 4,
  double: 8,
  bool: 1,
  byte: 1,
  char: 1,
  uchar: 1,
  short: 2,
  ushort: 2,
  int: 4,
  uint: 4,
  long: 8,
  ulong: 8,
  longlong: 8,
  ulonglong: 8,
  pointer: 8,
  size_t: 8,
  Object: 8 }

> os.alignof
{ int8: 1,
  uint8: 1,
  int16: 2,
  uint16: 2,
  int32: 4,
  uint32: 4,
  int64: 8,
  uint64: 8,
  float: 4,
  double: 8,
  bool: 1,
  char: 1,
  uchar: 1,
  short: 2,
  ushort: 2,
  int: 4,
  uint: 4,
  long: 8,
  ulong: 8,
  longlong: 8,
  ulonglong: 8,
  pointer: 8,
  size_t: 8,
  Object: 8 }

Hopefully pretty uncontroversial, but still needs docs and tests. Wanted to get notes/comments on implementation first though.

@TooTallNate TooTallNate added discuss Issues opened for discussions and feedbacks. ffi labels May 21, 2015
@Fishrock123 Fishrock123 added os Issues and PRs related to the os subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels May 21, 2015
@TooTallNate TooTallNate mentioned this pull request May 21, 2015
Closed
Fishrock123
Fishrock123 reviewed May 21, 2015
View reviewed changes
lib/os.js
@@ -54,3 +54,6 @@ if (binding.isBigEndian)
exports.endianness = function() { return 'BE'; };
else
exports.endianness = function() { return 'LE'; };

exports.sizeof = process.binding('sizeof');
exports.alignof = process.binding('alignof');
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to cache a return result here? (Or is that already what the c++ does?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The C++ layer already caches process.binding() results.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok cool, so long as this doesn't have to go through c++ each time it is accessed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lib/os.js also is only executed once (and the exports are cached), so yes, the values should basically be cached indefinitely.

@ChALkeR
Copy link
Member

ChALkeR commented May 21, 2015

What about long double?

@TooTallNate
Copy link
Contributor Author

@ChALkeR There is indeed an ffi_type_longdouble in libffi, so sure ya, I don't see any reason not to have it. Added in 3432054.

brendanashworth
brendanashworth reviewed May 21, 2015
View reviewed changes
src/alignof.cc
@@ -0,0 +1,68 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a specific reason why this is in a new file rather than src/node_os.cc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No particularly good reason, but I was planning on using process.binding('sizeof').pointer within buffer.js in order to add correct offset validation to the read/writePointer() functions. We could easily just require('os').sizeof.pointer instead though. I'm open either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay! I don't have much experience with the C++ anyways, but perhaps someone else may have an opinion on it.

@TooTallNate TooTallNate mentioned this pull request May 22, 2015
Closed
bnoordhuis
bnoordhuis reviewed May 22, 2015
View reviewed changes
src/alignof.cc
#define SET_ALIGNOF(name, type) \
struct s_##name { type a; }; \
exports->Set(FIXED_ONE_BYTE_STRING(env->isolate(), #name ), \
Uint32::NewFromUnsigned(env->isolate(), static_cast<uint32_t>(__alignof__(struct s_##name))));
Copy link
Member

Choose a reason for hiding this comment

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

Why don't you use alignof(type) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forget we're in C++ land sometimes :) Does that mean I can ditch the struct thing?

Copy link
Contributor

Choose a reason for hiding this comment

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

also, long line.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, no need for the struct.

@bnoordhuis
Copy link
Member

I would suggest merging the two .cc files. That would avoid most of the repetition and the potential for oversights when adding new types later on.

@bnoordhuis
Copy link
Member

Aside: I'm not sure if 'os' is a good place for this because it's not really specific to the operating system. Maybe introduce a new 'ffi' or 'ctypes' module?

trevnorris
trevnorris reviewed May 22, 2015
View reviewed changes
src/alignof.cc
SET_ALIGNOF(size_t, size_t);

// alignment of a Persistent handle to a JS object
SET_ALIGNOF(Object, Persistent<Object>);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain the need for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove it (copy and paste oversight).

But as an explanation, in ref I have readObject() and writeObject(), which allow you to create and write the memory address of a Persistent<Object> handle to a Buffer instance, for later retrieval. It was more black magic / experimental though, and probably unsafe for use in core. At least for now. It's not necessary for the main "FFI in core" mission anyways, so we could revisit the idea once all this big stuff is landed.

@TooTallNate
src: remove alignof.cc
f920871 
Will be merged with sizeof.cc
@TooTallNate
sizeof: move "alignof" logic to sizeof.cc
76dcf72 
This makes it easier in the future to add more types,
since they're now located in one location.
@TooTallNate
sizeof: asterisk to the left
35fbb99
This was referenced Jun 1, 2015
Closed
Closed
@TooTallNate
Copy link
Contributor Author

Closing due to @bnoordhuis recommendation of moving this to the ffi module. See #1865.

@TooTallNate TooTallNate closed this Jun 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
discuss Issues opened for discussions and feedbacks. os Issues and PRs related to the os subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@TooTallNate @ChALkeR @bnoordhuis @trevnorris @Fishrock123 @brendanashworth