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

Implement $Cxx.omitSchemas annotation #1502

Merged
merged 8 commits into from Jul 25, 2022
Merged

Implement $Cxx.omitSchemas annotation #1502

merged 8 commits into from Jul 25, 2022

Conversation

Vadmeme
Copy link
Contributor

@Vadmeme Vadmeme commented Jul 19, 2022

Implements a $Cxx.omitSchemas annotation that strips schema information from the binary as discussed in #1498

c++/src/capnp/c++.capnp Outdated Show resolved Hide resolved
c++/src/capnp/compiler/capnpc-c++.c++ Outdated Show resolved Hide resolved
@kentonv
Copy link
Member

kentonv commented Jul 20, 2022

Let's make sure this is tested. I think you could just apply the option to test-import2.capnp, to verify it still compiles. You might want to add some declarations in that file to cover enums, interfaces, etc. to make sure everything is covered.

@Vadmeme
Copy link
Contributor Author

Vadmeme commented Jul 21, 2022

I did the simple test of adding the annotation to the existing test-import2.capnp file
I also added some extra declarations to handle interfaces/enums/structs as per your suggestion.

Copy link
Member

@kentonv kentonv left a comment

Choose a reason for hiding this comment

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

Thanks for adding tests! Just a bunch of style nitpicks...

c++/src/capnp/c++.capnp Outdated Show resolved Hide resolved
c++/src/capnp/compiler/capnpc-c++.c++ Outdated Show resolved Hide resolved
c++/src/capnp/compiler/capnpc-c++.c++ Outdated Show resolved Hide resolved
c++/src/capnp/test-import2.capnp Outdated Show resolved Hide resolved
c++/src/capnp/test-import2.capnp Outdated Show resolved Hide resolved
c++/src/capnp/test-import2.capnp Outdated Show resolved Hide resolved
c++/src/capnp/test-import2.capnp Outdated Show resolved Hide resolved
c++/src/capnp/test-import2.capnp Outdated Show resolved Hide resolved
Vadmeme and others added 2 commits July 22, 2022 13:13
Co-authored-by: Kenton Varda <kenton@sandstorm.io>
@Vadmeme
Copy link
Contributor Author

Vadmeme commented Jul 22, 2022

Implemented style changes and added newlines

@kentonv kentonv merged commit 306904e into capnproto:master Jul 25, 2022
@kentonv
Copy link
Member

kentonv commented Jul 25, 2022

Looks good, thanks! Bazel failure seems unrelated.

@mikea
Copy link
Collaborator

mikea commented Jul 26, 2022

Bazel failure seems unrelated.

I think this change actually broke bazel build. I don't know why yet, but it passes on a previous commit. @kentonv @Vadmeme

@mikea
Copy link
Collaborator

mikea commented Jul 26, 2022

Other builds fail because of the same error:

/usr/bin/ld: CMakeFiles/capnp-heavy-tests.dir/test_capnp/capnp/test-import2.capnp.c++.o:(.data.rel.ro+0x0): undefined reference to `capnp::schemas::s_cfdb43ee5505db69'
/usr/bin/ld: CMakeFiles/capnp-heavy-tests.dir/test_capnp/capnp/test-import2.capnp.c++.o:(.data.rel.ro+0x8): undefined reference to `capnp::schemas::s_92adc979fd930df5'
make[5]: Leaving directory '/home/runner/work/capnproto/capnproto/c++/distcheck-cmake'
/usr/bin/ld: CMakeFiles/capnp-heavy-tests.dir/test_capnp/capnp/test-import2.capnp.c++.o:(.data.rel.ro+0x10): undefined reference to `capnp::schemas::s_9b550ca7cf5da1c1'
make[4]: Leaving directory '/home/runner/work/capnproto/capnproto/c++/distcheck-cmake'
/usr/bin/ld: CMakeFiles/capnp-heavy-tests.dir/test_capnp/capnp/test-import2.capnp.c++.o:(.data.rel.ro+0x18): undefined reference to `capnp::schemas::s_f2756f068e360761'
make[3]: Leaving directory '/home/runner/work/capnproto/capnproto/c++/distcheck-cmake'
make[2]: Leaving directory '/home/runner/work/capnproto/capnproto/c++/distcheck-cmake'
/usr/bin/ld: CMakeFiles/capnp-heavy-tests.dir/test_capnp/capnp/test-import2.capnp.c++.o:(.data.rel.ro+0x20): undefined reference to `capnp::schemas::s_951d576f783c952c'
make[1]: Leaving directory '/home/runner/work/capnproto/capnproto/c++'
/usr/bin/ld: CMakeFiles/capnp-heavy-tests.dir/test_capnp/capnp/test-import2.capnp.c++.o:(.data.rel.ro+0x28): undefined reference to `capnp::schemas::s_aa793a6361fa6e16'
/usr/bin/ld: CMakeFiles/capnp-heavy-tests.dir/test_capnp/capnp/test-import2.capnp.c++.o:(.data.rel.ro+0x30): undefined reference to `capnp::schemas::s_98e7c001808198cd'
/usr/bin/ld: CMakeFiles/capnp-heavy-tests.dir/test_capnp/capnp/test-import2.capnp.c++.o:(.data.rel.ro+0x38): undefined reference to `capnp::schemas::s_f6bd77f100ecb0ff'
collect2: error: ld returned 1 exit status

@Vadmeme
Copy link
Contributor Author

Vadmeme commented Jul 27, 2022

I personally am on windows and i dont think the current bazel build supports it, so i cannot run local testing and all my information is gathered from reading github action logs.

It appears that the problem is on the linker level as the object file already exists, if i had to guess it is because CAPNP_DECLARE_SCHEMA declares the schema values as extern so the linker tries to find them in external sources and cannot.

A possible workaround I can think of is instead of fully stripping schema data to emit a zero length array of data.

@kentonv
Copy link
Member

kentonv commented Jul 27, 2022

@mikea Sorry, when I looked at the log I was confused by all of the sign-compare warnings and didn't see the link errors. We should disable that warning (it's disabled in the other builds).

All of the CI builds other than Bazel passed. (Well, and MinGW which has been broken for a while.)

@kentonv
Copy link
Member

kentonv commented Jul 27, 2022

Weirdly the cmake build fails for me locally (although the automake build does not, and obviously neither failed in CI).

kentonv added a commit that referenced this pull request Jul 27, 2022
@kentonv
Copy link
Member

kentonv commented Jul 27, 2022

I've reverted the change for now.

@Vadmeme we need a version that actually removes the declarations from the header file, I think.

@Vadmeme
Copy link
Contributor Author

Vadmeme commented Jul 27, 2022

I looked into simply removing the CAPNP_DECLARE_SCHEMA from the header however this caused alot of problems with the schemas being referenced by other parts of capnp that i feel would be a bad idea to mess with.

An alternative is what i have suggested above of replacing the buffer with schema data with a zero sized placeholder as shown below. I have already implemented this and it seems to work and pass all test cases and should not cause any linker issues.

However before i make another PR id like to make sure this workaround is something you would be open too.

static const ::capnp::_::AlignedData<0> b_f6bd77f100ecb0ff = {{}};
::capnp::word const* const bp_f6bd77f100ecb0ff = b_f6bd77f100ecb0ff.words;
#if !CAPNP_LITE
static const ::capnp::_::RawSchema* const d_f6bd77f100ecb0ff[] = {
  &s_a0a8f314b80b63fd,
  &s_bc55b08b672b5d97,
  &s_e682ab4cf923a417,
};
static const uint16_t m_f6bd77f100ecb0ff[] = {1, 2, 0};
static const uint16_t i_f6bd77f100ecb0ff[] = {0, 1, 2};
const ::capnp::_::RawSchema s_f6bd77f100ecb0ff = {
  0xf6bd77f100ecb0ff, b_f6bd77f100ecb0ff.words, 0, d_f6bd77f100ecb0ff, m_f6bd77f100ecb0ff,
  3, 3, i_f6bd77f100ecb0ff, nullptr, nullptr, { &s_f6bd77f100ecb0ff, nullptr, nullptr, 0, 0, nullptr }
};
#endif  // !CAPNP_LITE

@kentonv
Copy link
Member

kentonv commented Jul 27, 2022

I don't like the empty buffer approach as it means any attempt to use the schemas (e.g. stringifying a message) would compile just fine and fail at runtime. I'd rather the error be at compile time.

@Vadmeme
Copy link
Contributor Author

Vadmeme commented Jul 27, 2022

The only other option i see is manually editing everything that exposed an encodedSchema method in generated-header-support.h and adding a "_STRIPPED" variant to that exposes encodedSchema that is just a static_assert(false) or something similar.

@kentonv
Copy link
Member

kentonv commented Jul 27, 2022

Ideally I think we don't want to declare encodedSchema() at all if there's no schema. Does anything break if we omit it entirely?

Maybe what we need to do here is split the macros like CAPNP_DECLARE_STRUCT_HEADER into one macro that declares the non-schema-related stuff, and another that declares the schema stuff. When omitSchemas is enabled, we only use the former macro.

@Vadmeme
Copy link
Contributor Author

Vadmeme commented Jul 27, 2022

What you suggested would be ideal but i personally do not know enough about the inner workings of the generated headers to implement it yet.

Nothing SHOULD break if encodedSchema() does not exist as its only use is in schemaProto() and thats only used if you use schemas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants