fix(common): fewer crashes with dynamic loading #7512
Conversation
`absl::any` does not support dynamically loaded libraries: it supports `.so` libraries, it does not support libraries loaded with `dlopen()` or a similar call. For our needs, it is easy to implement type-erasure in `google::cloud::Options`.
google-cla
bot
added
the
cla: yes
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov Report
@@ Coverage Diff @@
## main #7512 +/- ##
==========================================
- Coverage 93.66% 93.66% -0.01%
==========================================
Files 1361 1361
Lines 118086 118101 +15
==========================================
+ Hits 110606 110619 +13
- Misses 7480 7482 +2
Continue to review full report at Codecov.
|
| p = m_.emplace(typeid(T), absl::make_unique<Data<T>>(std::move(value))) | ||
| .first; | ||
| } | ||
| auto* v = p->second->data_address(); |
There was a problem hiding this comment.
Consider whether you'd like to factor these two lines (w/ the reinterpret_cast) into a private helper function template in this class, maybe like:
template <typename T, typename Holder>
T& GetValueReference(Holder&& h) {
auto* v = std::forward<Holder>(h)->data_address();
return *reinterpret_cast<T*>(v);
}IFF that works (it's untested, so maybe doesn't even compile), would that let us isolate the slightly subtle reinterpret cast stuff into a single place. Your call about whether you even think this is an improvement. I'm happy w/ whatever you decide.
There was a problem hiding this comment.
Hmm... I am going to pass. One is const-qualified, the other is not. I am sure we could make that work, not sure it is worth the trouble.
There was a problem hiding this comment.
Passing SGTM. But just to clarify the suggestion, the use of std::forward would avoid the const-qualification issues. The following works:
diff --git a/google/cloud/options.h b/google/cloud/options.h
index a9d4856f2..0f06fe9a2 100644
--- a/google/cloud/options.h
+++ b/google/cloud/options.h
@@ -165,8 +165,7 @@ class Options {
static auto const* const kDefaultValue = new ValueTypeT<T>{};
auto const it = m_.find(typeid(T));
if (it == m_.end()) return *kDefaultValue;
- auto const* value = it->second->data_address();
- return *reinterpret_cast<ValueTypeT<T> const*>(value);
+ return GetValueReference<ValueTypeT<T> const>(*it->second);
}
/**
@@ -196,8 +195,7 @@ class Options {
p = m_.emplace(typeid(T), absl::make_unique<Data<T>>(std::move(value)))
.first;
}
- auto* v = p->second->data_address();
- return *reinterpret_cast<ValueTypeT<T>*>(v);
+ return GetValueReference<ValueTypeT<T>>(*p->second);
}
private:
@@ -214,6 +212,11 @@ class Options {
virtual std::unique_ptr<DataHolder> clone() const = 0;
};
+ template <typename T, typename Holder>
+ static T& GetValueReference(Holder&& h) {
+ return *reinterpret_cast<T*>(std::forward<Holder>(h).data_address());
+ }
+
// The data holder for all the option values.
template <typename T>
class Data : public DataHolder {I don't actually care about this change; just wanted to see if it worked like I was thinking.
absl::anydoes not support dynamically loaded libraries: it supports.solibraries, it does not support libraries loaded withdlopen()ora similar call. For our needs, it is easy to implement type-erasure in
google::cloud::Options.Fixes #7505
This change is