rocksdb/options/customizable.cc
Peter Dillinger 54cb9c77d9 Prefer static_cast in place of most reinterpret_cast (#12308)
Summary:
The following are risks associated with pointer-to-pointer reinterpret_cast:
* Can produce the "wrong result" (crash or memory corruption). IIRC, in theory this can happen for any up-cast or down-cast for a non-standard-layout type, though in practice would only happen for multiple inheritance cases (where the base class pointer might be "inside" the derived object). We don't use multiple inheritance a lot, but we do.
* Can mask useful compiler errors upon code change, including converting between unrelated pointer types that you are expecting to be related, and converting between pointer and scalar types unintentionally.

I can only think of some obscure cases where static_cast could be troublesome when it compiles as a replacement:
* Going through `void*` could plausibly cause unnecessary or broken pointer arithmetic. Suppose we have
`struct Derived: public Base1, public Base2`.  If we have `Derived*` -> `void*` -> `Base2*` -> `Derived*` through reinterpret casts, this could plausibly work (though technical UB) assuming the `Base2*` is not dereferenced. Changing to static cast could introduce breaking pointer arithmetic.
* Unnecessary (but safe) pointer arithmetic could arise in a case like `Derived*` -> `Base2*` -> `Derived*` where before the Base2 pointer might not have been dereferenced. This could potentially affect performance.

With some light scripting, I tried replacing pointer-to-pointer reinterpret_casts with static_cast and kept the cases that still compile. Most occurrences of reinterpret_cast have successfully been changed (except for java/ and third-party/). 294 changed, 257 remain.

A couple of related interventions included here:
* Previously Cache::Handle was not actually derived from in the implementations and just used as a `void*` stand-in with reinterpret_cast. Now there is a relationship to allow static_cast. In theory, this could introduce pointer arithmetic (as described above) but is unlikely without multiple inheritance AND non-empty Cache::Handle.
* Remove some unnecessary casts to void* as this is allowed to be implicit (for better or worse).

Most of the remaining reinterpret_casts are for converting to/from raw bytes of objects. We could consider better idioms for these patterns in follow-up work.

I wish there were a way to implement a template variant of static_cast that would only compile if no pointer arithmetic is generated, but best I can tell, this is not possible. AFAIK the best you could do is a dynamic check that the void* conversion after the static cast is unchanged.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/12308

Test Plan: existing tests, CI

Reviewed By: ltamasi

Differential Revision: D53204947

Pulled By: pdillinger

fbshipit-source-id: 9de23e618263b0d5b9820f4e15966876888a16e2
2024-02-07 10:44:11 -08:00

134 lines
4.4 KiB
C++

// Copyright (c) 2011-present, Facebook, Inc. All rights reserved.
// This source code is licensed under both the GPLv2 (found in the
// COPYING file in the root directory) and Apache 2.0 License
// (found in the LICENSE.Apache file in the root directory).
#include "rocksdb/customizable.h"
#include <sstream>
#include "options/options_helper.h"
#include "port/port.h"
#include "rocksdb/convenience.h"
#include "rocksdb/status.h"
#include "rocksdb/utilities/options_type.h"
#include "util/string_util.h"
namespace ROCKSDB_NAMESPACE {
std::string Customizable::GetOptionName(const std::string& long_name) const {
const std::string& name = Name();
size_t name_len = name.size();
if (long_name.size() > name_len + 1 &&
long_name.compare(0, name_len, name) == 0 &&
long_name.at(name_len) == '.') {
return long_name.substr(name_len + 1);
} else {
return Configurable::GetOptionName(long_name);
}
}
std::string Customizable::GenerateIndividualId() const {
std::ostringstream ostr;
ostr << Name() << "@" << static_cast<const void*>(this) << "#"
<< port::GetProcessID();
return ostr.str();
}
Status Customizable::GetOption(const ConfigOptions& config_options,
const std::string& opt_name,
std::string* value) const {
if (opt_name == OptionTypeInfo::kIdPropName()) {
*value = GetId();
return Status::OK();
} else {
return Configurable::GetOption(config_options, opt_name, value);
}
}
std::string Customizable::SerializeOptions(const ConfigOptions& config_options,
const std::string& prefix) const {
std::string result;
std::string parent;
std::string id = GetId();
if (!config_options.IsShallow() && !id.empty()) {
parent = Configurable::SerializeOptions(config_options, "");
}
if (parent.empty()) {
result = id;
} else {
result.append(prefix);
result.append(OptionTypeInfo::kIdPropName());
result.append("=");
result.append(id);
result.append(config_options.delimiter);
result.append(parent);
}
return result;
}
bool Customizable::AreEquivalent(const ConfigOptions& config_options,
const Configurable* other,
std::string* mismatch) const {
if (config_options.sanity_level > ConfigOptions::kSanityLevelNone &&
this != other) {
const Customizable* custom = static_cast<const Customizable*>(other);
if (custom == nullptr) { // Cast failed
return false;
} else if (GetId() != custom->GetId()) {
*mismatch = OptionTypeInfo::kIdPropName();
return false;
} else if (config_options.sanity_level >
ConfigOptions::kSanityLevelLooselyCompatible) {
bool matches =
Configurable::AreEquivalent(config_options, other, mismatch);
return matches;
}
}
return true;
}
Status Customizable::GetOptionsMap(
const ConfigOptions& config_options, const Customizable* customizable,
const std::string& value, std::string* id,
std::unordered_map<std::string, std::string>* props) {
Status status;
if (value.empty() || value == kNullptrString) {
*id = "";
props->clear();
} else if (customizable != nullptr) {
status =
Configurable::GetOptionsMap(value, customizable->GetId(), id, props);
if (status.ok() && customizable->IsInstanceOf(*id)) {
// The new ID and the old ID match, so the objects are the same type.
// Try to get the existing options, ignoring any errors
ConfigOptions embedded = config_options;
embedded.delimiter = ";";
std::string curr_opts;
if (customizable->GetOptionString(embedded, &curr_opts).ok()) {
std::unordered_map<std::string, std::string> curr_props;
if (StringToMap(curr_opts, &curr_props).ok()) {
props->insert(curr_props.begin(), curr_props.end());
}
}
}
} else {
status = Configurable::GetOptionsMap(value, "", id, props);
}
return status;
}
Status Customizable::ConfigureNewObject(
const ConfigOptions& config_options, Customizable* object,
const std::unordered_map<std::string, std::string>& opt_map) {
Status status;
if (object != nullptr) {
status = object->ConfigureFromMap(config_options, opt_map);
} else if (!opt_map.empty()) {
status = Status::InvalidArgument("Cannot configure null object ");
}
return status;
}
} // namespace ROCKSDB_NAMESPACE