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

Re-enable RTTI (needed in order to subclass Source, etc.) #144

Closed
wants to merge 1 commit into from

Conversation

tserong
Copy link

@tserong tserong commented Oct 27, 2021

Commit c98344f in snappy 1.1.9 disabled RTTI, which means the snappy library no longer exports typeinfo for snappy::Source, snappy::Sink, ..., so users of the library can't subclass these classes anymore.

Here's a trivial reproducer:

  #include <snappy-sinksource.h>
  class MySource : snappy::Source {
  public:
    size_t Available() const override { return 0; }
    const char *Peek(size_t *len) override { return NULL; }
    void Skip(size_t n) override {}
  };
  int main(int argc, char **argv) {
    MySource m;
    return 0;
  }

Try g++ -o snappy-test ./snappy-test.cc -lsnappy with the above and the linker will fail with "undefined reference to `typeinfo for snappy::Source'" if RTTI was disabled in the snappy build.

Commit c98344f in snappy 1.1.9 disabled RTTI, which means the
snappy library no longer exports typeinfo for snappy::Source,
snappy::Sink, ..., so users of the library can't subclass these
classes anymore.

Here's a trivial reproducer:

  #include <snappy-sinksource.h>
  class MySource : snappy::Source {
  public:
    size_t Available() const override { return 0; }
    const char *Peek(size_t *len) override { return NULL; }
    void Skip(size_t n) override {}
  };
  int main(int argc, char **argv) {
    MySource m;
    return 0;
  }

Try `g++ -o snappy-test ./snappy-test.cc -lsnappy` with the above
and the linker will fail with "undefined reference to `typeinfo
for snappy::Source'" if RTTI was disabled in the snappy build.
@google-cla google-cla bot added the cla: yes label Oct 27, 2021
@pwnall
Copy link
Member

pwnall commented Nov 14, 2021

Sorry, I am not going to accept this PR.

The current CMake configuration reflects the supported build setup for this library, and what runs in CI. RTTI is disabled in Google Chrome, which is the largest testing vehicle for Snappy.

@pwnall pwnall closed this Nov 14, 2021
@tserong
Copy link
Author

tserong commented Nov 15, 2021

Would you mind bumping the soname version then, as disabling RTTI breaks the ABI? A similar issue was raised against leveldb (google/leveldb#927)

clrpackages pushed a commit to clearlinux-pkgs/snappy that referenced this pull request Apr 7, 2022
This patch addresses an issue with Ceph
https://tracker.ceph.com/issues/53060. There have been multiple patches
to upstream snappy, but they have all been rejected and the reasoning
has always been that snappy would like to be consistent in their build
flags with chrome as mentioned at
google/snappy#144 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants