Differently aligned fields may make inheritance not work properly.

[jsgf]

This input:

namespace
{
  template < typename _CharT, typename = _CharT > class basic_ostream;
  template < typename, typename > class basic_ios
  {
  };
template < typename _CharT, typename _Traits > class basic_ostream:virtual basic_ios < _CharT,
    _Traits
    >
  {
  };
  class strstreambuf
  {
  };
  class ostrstream:basic_ostream < char >
  {
    strstreambuf _M_buf;
  };
  class LogMessage
  {
    class LogStream:ostrstream
    {
      int ctr_;
    };
  };
}

Command:

bindgen --output logging.rs --no-unstable-rust --whitelist-type '.*::LogMessage' --whitelist-type '.*::LogSeverity' --whitelist-type '.*::int64' --blacklist-type std::allocator --blacklist-type std::allocator_traits --blacklist-type std::__alloc_traits --opaque-type std::string --opaque-type std::basic_fbstring --opaque-type std::vector --opaque-type std::basic_ios --opaque-type std::basic_ostream --opaque-type std::basic_streambuf --generate functions,methods,types test.i -- -std=gnu++14 -x c++

Generates rust code which fails a unit test when built with --test:

thread 'bindgen_test_layout__bindgen_mod_id_1_LogMessage_LogStream' panicked at 'assertion failed: `(left == right)` (left: `24`, right: `16`)', test.rs:66

This is the generated output is here: https://gist.github.com/jsgf/ddcc0db8b239b781d0590199cbe2ec80

[emilio]

Gah, virtual inheritance... We don't have any code for handling it and we're missing the base class pointer.

Thanks for reporting, will investigate how to extract this info from clang.

[emilio]

After digging a bit more I don't think this is due to virtual inheritance anymore (it sort of is, because there was a bug in our handling of virtual inheritance, but it's not the root cause of this one I believe).

[emilio]

More minimized:

class basic_ios { };

class basic_ostream: virtual basic_ios { };

class ostrstream:basic_ostream {
  int _M_buf;
};

class LogStream:ostrstream {
  int ctr_;
};

I think this is because how alignment is done by clang and rustc, mainly, ostrstream is 16-byte aligned on 64 bits, but clang still lays out the int crt_ field contiguously (because it's a base class), while rustc lays it out in the next 16-byte slot because we represent it as a field.

[emilio]

I'll grab a debugger and inspect the fields, but I suspect that offsetof(LogStream, crt_) is going to be 8-byte aligned, but not 16-byte aligned (that is, contiguous to _M_buf).

[emilio]

Yup, not related to virtual inheritance at all, more minimized:

class ostrstream {
  void* ptr;
  int _M_buf;
};

class LogStream:ostrstream {
  int ctr_;
};

I'm not sure what's the best way to solve this unfortunately... Ideally having some kind of annotation on rustc, but the short-term solutions I'm thinking about aren't pretty (we wouldn't have easy access to base classes as we have now).

[emilio]

I guess the easiest way to solve this is inlining all the base class fields, then add helper methods to transmute as base classes, instead of the way we represent inheritance now.

It's a hard-ish patch though, and I'm not sure I'll have the time to do it before exams are over :)

[emilio]

cc @fitzgen, can you think of any better option?

[emilio]

Ugh, making that approach work with template parameters will need implementing template parameter resolution logic I really don't want to write :(

[emilio]

I guess it's the only sensible option though, more ideas appreciated.

[emilio]

Actually, this is going to need some rust compiler magic I believe, otherwise examples like this (that currently work) would stop working if we just inline the fields:

class WordAligned {
  void* ptr;
  int foo;
};

class WordAlignedButWithDifferentlyAlignedFirstField {
  int foo;
  void* ptr;
};

class Test : public WordAligned,
             public WordAlignedButWithDifferentlyAlignedFirstField
{};

[emilio]

It seems that the logic is a bit more complicated, and clang only packs bits on the last base class. That is, this test case still fails:

class WordAligned {
  void* ptr;
  int foo;
};

class WordAlignedButWithDifferentlyAlignedFirstField {
  int foo;
  void* ptr;
  int bar;
};

class Test : public WordAligned,
             public WordAlignedButWithDifferentlyAlignedFirstField
{
  int baz;
};

[emilio]

TL;DR: I think the sensible way to do this is inlining the last, and only the last base class members inside a given struct.

That's going to be slightly annoying to implement, but might be doable.

[fitzgen]

Interesting... It's really only the last one?

That is annoying because it means we can't always inline, which would be easier (if we have to do any inlining at all). If all bases were packed, we could have a single path. I guess we could add padding ourselves...

[emilio]

Upon reflection, it has nothing to do with whether it'd be the last field or not, but with whether the class would be properly aligned if it was packed inside.

[fitzgen]

That makes much more sense to me.

[fitzgen]

And means that the always-inline-base-members-and-generate-upcast-methods approach is more reasonable, no?

[emilio]

You can't just inline base members because you may get incorrectly padded fields, like in the test case I wrote above, so we need to, at least, figure out which padding to add at (which is alignment - first_field_alignment) I think.

Also, we need to track template parameters and properly resolve them, which is going to be somewhat annoying.

[emilio]

https://bugzilla.mozilla.org/show_bug.cgi?id=1363375 seems to indicate that MSVC doesn't follow the same rule here, which makes it much more fun to fix \o/

[matklad]

Not sure if this is relevant, but looks like the visibility of individual fields matters here as well: https://gist.github.com/matklad/9471c7dc2388822e373d9371f728817e =/

[alexreg]

Any update on this lately?

[dylanede]

Just bumped into this problem. Anyone got a workaround?

[sagudev]

In https://github.com/sagudev/mozjs/commit/fac8536d44538c2cf2ded3d4eedfd6e0314a20b6 I add to class __attribute__ ((__packed__)) and the problem was solved.

[zopsicle]

Another workaround is to add padding fields to the base class for suitable alignment of the first field of the derived class. For instance, with the above example:

 class ostrstream {
   void* ptr;
   int _M_buf;
+  int rustBindgenPadding;
 };

 class LogStream:ostrstream {
   int ctr_;
 };

Obviously this requires the C++ code to be recompiled, which may or may not be a problem.