Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2957753002:
No compliation-flag-dependent members in CriticalSection (Closed)
DescriptionNo compliation-flag-dependent members in CriticalSection
Having members in a class which only exist when certain compliation flags are turned on (unless relating to the target platform) means that those flags must be the same when compiling the module as when including its headers from other modules. This means that code outside of WebRTC runs the risk of misjudging the size of an rtc::CriticalSection, or any class which has an rtc::CriticalSection as a member. (This rule is applied recursively.) If a mismatch occurs, memory corruption is likely.
Having discussed this a bit, we have decided that the simplest solution is probably the best - always define those members, even when compilation flags (namely, CS_DEBUG_CHECKS) render it unused.
BUG=webrtc:7867
Review-Url: https://codereview.webrtc.org/2957753002
Cr-Commit-Position: refs/heads/master@{#18811}
Committed: https://chromium.googlesource.com/external/webrtc/+/d6e9466e7ef8736c5dd4c736c9732506b1b70c1e
Patch Set 1 #
Total comments: 7
Patch Set 2 : . #
Total comments: 4
Patch Set 3 : . #
Messages
Total messages: 21 (7 generated)
eladalon@webrtc.org changed reviewers: + danilchap@webrtc.org, nisse@webrtc.org, stefan@webrtc.org
PTAL (I'll add owners once we have consensus.) https://codereview.webrtc.org/2957753002/diff/1/webrtc/base/criticalsection.cc File webrtc/base/criticalsection.cc (right): https://codereview.webrtc.org/2957753002/diff/1/webrtc/base/criticalsection.c... webrtc/base/criticalsection.cc:23: #elif defined(WEBRTC_POSIX) mutex_ does not really exist otherwise; would be a compilation error if we ever have a non-WEBRTC_WIN build that's also not WEBRTC_POSIX. I don't know if that's likely, but might as well have the structure here mimic that of the header. So we have two options: 1. Change the header to not depend on WEBRTC_POSIX for this. 2. Change everywhere that mute_ is accessed, and make sure that WEBRTC_POSIX. I prefer the second option. Wdyt?
https://codereview.webrtc.org/2957753002/diff/1/webrtc/base/criticalsection.cc File webrtc/base/criticalsection.cc (right): https://codereview.webrtc.org/2957753002/diff/1/webrtc/base/criticalsection.c... webrtc/base/criticalsection.cc:23: #elif defined(WEBRTC_POSIX) On 2017/06/26 12:35:22, eladalon wrote: > mutex_ does not really exist otherwise; would be a compilation error if we ever > have a non-WEBRTC_WIN build that's also not WEBRTC_POSIX. I don't know if that's > likely, but might as well have the structure here mimic that of the header. > > So we have two options: > 1. Change the header to not depend on WEBRTC_POSIX for this. > 2. Change everywhere that mute_ is accessed, and make sure that WEBRTC_POSIX. > > I prefer the second option. Wdyt? If you do it this way, also add an #else #error Platform not supported #endif to not fail silently on unexpected platforms. For the main question, I think the important thing is to get a clear compilation error for unsupported platforms, but it doesn't matter very much if the the message says "error unsupported platform" or "mutex_ is not defined". https://codereview.webrtc.org/2957753002/diff/1/webrtc/base/criticalsection.c... webrtc/base/criticalsection.cc:38: RTC_UNUSED(thread_); // Depends on CS_DEBUG_CHECKS. Drop this comment. Instead, add a comment in the header saying that these member variables are used for DCHECKs only. https://codereview.webrtc.org/2957753002/diff/1/webrtc/base/criticalsection.h File webrtc/base/criticalsection.h (right): https://codereview.webrtc.org/2957753002/diff/1/webrtc/base/criticalsection.h... webrtc/base/criticalsection.h:82: mutable PlatformThreadRef owning_thread_; As discussed offline, it would make sense to me to unify owning_thread_/thread_ and recursion_/recursion_count_, but in a separate cl.
Please take another look. :-) https://codereview.webrtc.org/2957753002/diff/1/webrtc/base/criticalsection.cc File webrtc/base/criticalsection.cc (right): https://codereview.webrtc.org/2957753002/diff/1/webrtc/base/criticalsection.c... webrtc/base/criticalsection.cc:23: #elif defined(WEBRTC_POSIX) On 2017/06/26 12:53:09, nisse-webrtc wrote: > On 2017/06/26 12:35:22, eladalon wrote: > > mutex_ does not really exist otherwise; would be a compilation error if we > ever > > have a non-WEBRTC_WIN build that's also not WEBRTC_POSIX. I don't know if > that's > > likely, but might as well have the structure here mimic that of the header. > > > > So we have two options: > > 1. Change the header to not depend on WEBRTC_POSIX for this. > > 2. Change everywhere that mute_ is accessed, and make sure that WEBRTC_POSIX. > > > > I prefer the second option. Wdyt? > > If you do it this way, also add an > > #else > #error Platform not supported > #endif > > to not fail silently on unexpected platforms. > > For the main question, I think the important thing is to get a clear compilation > error for unsupported platforms, but it doesn't matter very much if the the > message says "error unsupported platform" or "mutex_ is not defined". Added throughout the module. https://codereview.webrtc.org/2957753002/diff/1/webrtc/base/criticalsection.c... webrtc/base/criticalsection.cc:38: RTC_UNUSED(thread_); // Depends on CS_DEBUG_CHECKS. On 2017/06/26 12:53:09, nisse-webrtc wrote: > Drop this comment. Instead, add a comment in the header saying that these member > variables are used for DCHECKs only. Done. https://codereview.webrtc.org/2957753002/diff/1/webrtc/base/criticalsection.h File webrtc/base/criticalsection.h (right): https://codereview.webrtc.org/2957753002/diff/1/webrtc/base/criticalsection.h... webrtc/base/criticalsection.h:82: mutable PlatformThreadRef owning_thread_; On 2017/06/26 12:53:09, nisse-webrtc wrote: > As discussed offline, it would make sense to me to unify owning_thread_/thread_ > and recursion_/recursion_count_, but in a separate cl. I'll leave this until we take care of the TODO at the top of the .cc file; I'm afraid to change such critical (no pun intended) code while it's this unreadable.
lgtm, with some very minor nits. https://codereview.webrtc.org/2957753002/diff/20001/webrtc/base/criticalsecti... File webrtc/base/criticalsection.cc (right): https://codereview.webrtc.org/2957753002/diff/20001/webrtc/base/criticalsecti... webrtc/base/criticalsection.cc:40: #else // !defined(WEBRTC_WIN) && !defined(WEBRTC_POSIX) I'd prefer dropping this comment, it adds litte information to the error message on the line below, and there are no comments on the other (more interesting) #elses and #endifs in the file. https://codereview.webrtc.org/2957753002/diff/20001/webrtc/base/criticalsecti... File webrtc/base/criticalsection.h (right): https://codereview.webrtc.org/2957753002/diff/20001/webrtc/base/criticalsecti... webrtc/base/criticalsection.h:89: #error Unsupported platform. For consistency, consider indenting like "# error" here and below.
https://codereview.webrtc.org/2957753002/diff/20001/webrtc/base/criticalsecti... File webrtc/base/criticalsection.cc (right): https://codereview.webrtc.org/2957753002/diff/20001/webrtc/base/criticalsecti... webrtc/base/criticalsection.cc:40: #else // !defined(WEBRTC_WIN) && !defined(WEBRTC_POSIX) On 2017/06/26 13:49:46, nisse-webrtc wrote: > I'd prefer dropping this comment, it adds litte information to the error message > on the line below, and there are no comments on the other (more interesting) > #elses and #endifs in the file. I've removed, though my own preference would have been to keep. https://codereview.webrtc.org/2957753002/diff/20001/webrtc/base/criticalsecti... File webrtc/base/criticalsection.h (right): https://codereview.webrtc.org/2957753002/diff/20001/webrtc/base/criticalsecti... webrtc/base/criticalsection.h:89: #error Unsupported platform. On 2017/06/26 13:49:46, nisse-webrtc wrote: > For consistency, consider indenting like "# error" here and below. Done.
Description was changed from ========== No compliation-flag-dependent members in CriticalSection Having members in a class which only exist when certain compliation flags are turned on (unless relating to the target platform) means that those flags must be the same when compiling the module as when including its headers from other modules. This means that code outside of WebRTC runs the risk of misjudging the size of an rtc::CriticalSection, or any class which has an rtc::CriticalSection as a member. (This rule is applied recursively.) If a mismatch occurs, memory corruption is likely. Having discussed this a bit, we have decided that the simplest solution is probably the best - always define those members, even when compilation flags (namely, CS_DEBUG_CHECKS) render it unused. BUG=webrtc:7867 ========== to ========== No compliation-flag-dependent members in CriticalSection Having members in a class which only exist when certain compliation flags are turned on (unless relating to the target platform) means that those flags must be the same when compiling the module as when including its headers from other modules. This means that code outside of WebRTC runs the risk of misjudging the size of an rtc::CriticalSection, or any class which has an rtc::CriticalSection as a member. (This rule is applied recursively.) If a mismatch occurs, memory corruption is likely. Having discussed this a bit, we have decided that the simplest solution is probably the best - always define those members, even when compilation flags (namely, CS_DEBUG_CHECKS) render it unused. BUG=webrtc:7867 ==========
eladalon@webrtc.org changed reviewers: + kwiberg@webrtc.org
I'm not sure I agree with the basic premise of this CL. AFAIK we don't support, nor intend to support, building parts of WebRTC with debug flags and parts without. Was this prompted by a problem you observed?
On 2017/06/27 10:33:57, kwiberg-webrtc wrote: > I'm not sure I agree with the basic premise of this CL. AFAIK we don't support, > nor intend to support, building parts of WebRTC with debug flags and parts > without. I agree with that. But the use case where I think this matters is for applications which include webrtc header files and then link to webrtc .a and .so files. We have little control of the preprocessor defines used in the applications' compilation units, and things are likely to break in hard-to-debug ways if its inclusion of criticalsection.h uses inconsistent defines compared to webrtc compilation units. This is assuming that criticalsection.h is considered a public header file, or included indirectly by public header files, and I guess it is? Also note that the default value of RTC_DCHECK_IS_ON depends on NDEBUG (part of the assert facility of the standard C library). And the C library supports or even encourages setting of NDEBUG to different values in different files. One can even use different setting in different parts of the same file by including assert.h multiple times. IMO, we really shouldn't let our ABI depend on the value of NDEBUG.
On 2017/06/27 12:11:54, nisse-webrtc wrote: > On 2017/06/27 10:33:57, kwiberg-webrtc wrote: > > I'm not sure I agree with the basic premise of this CL. AFAIK we don't > support, > > nor intend to support, building parts of WebRTC with debug flags and parts > > without. > > I agree with that. > > But the use case where I think this matters is for applications which include > webrtc header files and then link to webrtc .a and .so files. We have little > control of the preprocessor defines used in the applications' compilation units, > and things are likely to break in hard-to-debug ways if its inclusion of > criticalsection.h uses inconsistent defines compared to webrtc compilation > units. Well, they have always compiled that .a or .so file themselves, right? So they should have control over the build flags. > This is assuming that criticalsection.h is considered a public header file, or > included indirectly by public header files, and I guess it is? Yes, I presume so. This sort of thing will be easier to see once all transitively public header files are in api/. > Also note that the default value of RTC_DCHECK_IS_ON depends on NDEBUG (part of > the assert facility of the standard C library). And the C library supports or > even encourages setting of NDEBUG to different values in different files. One > can even use different setting in different parts of the same file by including > assert.h multiple times. > > IMO, we really shouldn't let our ABI depend on the value of NDEBUG. That doesn't sound wrong. I wonder if the right solution is to not let RTC_DCHECK_IS_ON depend on NDEBUG, then, but to set it based on a GN parameter. That way, it would become clear that you can't mix values in one build. I'm not too happy with the alternative suggested by this CL, because (1) it prevents us from adding overhead-free debugging state in a whole lot of places, and (2) there seems to be no obvious way to prevent it automatically.
On 2017/06/27 12:43:04, kwiberg-webrtc wrote: > Well, they have always compiled that .a or .so file themselves, right? So they > should have control over the build flags. Not necessarily, if they use the "sdk"s we're trying to provide at least on ios or android, they'd just get a set of internally consistent library files and headers. Not sure how they're typically used, but it seems desirably to me that they can rebuild their app in debug mode or release mode, possibly flipping NDEBUG in their compilation units, without switching to a different set of webrtc library files at link time. > I wonder if the right solution is to not let RTC_DCHECK_IS_ON depend on NDEBUG, > then, but to set it based on a GN parameter. That way, it would become clear > that you can't mix values in one build. If we really want to allow ABI differences between debug and release builds, we shouldn't depend on NDEBUG. And we should probably export the setting from GN into a generated header file, say, api/build-config.h, included by header files, and part of the sdk packages, so that we get the same values both in the webrtc build and in applications. > I'm not too happy with the alternative suggested by this CL, because (1) it > prevents us from adding overhead-free debugging state in a whole lot of places, > and (2) there seems to be no obvious way to prevent it automatically. I think we can expect some real pain from making the ABI depend on debug vs release settings. So it's not an obvios trade-off. Perhaps you should check with Björn about the details of the similar problems we got with BWE logging and related defines a while ago. At least, I don't think any third_party applications were involved in that case.
On 2017/06/27 13:12:45, nisse-webrtc wrote: > On 2017/06/27 12:43:04, kwiberg-webrtc wrote: > > Well, they have always compiled that .a or .so file themselves, right? So they > > should have control over the build flags. > > Not necessarily, if they use the "sdk"s we're trying to provide at least on ios > or android, > they'd just get a set of internally consistent library files and headers. Ah, right. Those. > Not sure how they're typically used, but it seems desirably to me that they can > rebuild their app in debug mode or release mode, possibly flipping NDEBUG in > their compilation units, without switching to a different set of webrtc library > files at link time. Yes, definitely. > > I wonder if the right solution is to not let RTC_DCHECK_IS_ON depend on > NDEBUG, > > then, but to set it based on a GN parameter. That way, it would become clear > > that you can't mix values in one build. > > If we really want to allow ABI differences between debug and release builds, we > shouldn't depend on NDEBUG. I agree. > And we should probably export the setting from GN > into a generated header file, say, api/build-config.h, included by header files, > and part of the sdk packages, so that we get the same values both in the webrtc > build and in applications. This goes for all other GN settings that affect the ABI too, presumably? > > I'm not too happy with the alternative suggested by this CL, because (1) it > > prevents us from adding overhead-free debugging state in a whole lot of > places, > > and (2) there seems to be no obvious way to prevent it automatically. > > I think we can expect some real pain from making the ABI depend on debug vs > release settings. So it's not an obvious trade-off. Really? Even after doing the build-config.h thing that you suggested? > Perhaps you should check with > Björn about the details of the similar problems we got with BWE logging and > related defines a while ago. At least, I don't think any third_party > applications were involved in that case.
On 2017/06/27 13:25:05, kwiberg-webrtc wrote: > This goes for all other GN settings that affect the ABI too, presumably? Yes, at least, all settings which are controlled via the preprocessor. Not sure how to deal with things like exceptions and rtti; if there are some corresponding predefines set by the compiler, build-config.h could at least check those and produce a warning or error if the headers are used with incompatible abi settings. > Really? Even after doing the build-config.h thing that you suggested? build-config.h should definitely help. But I'm not sure if it would have solved Björn's problem, I don't remember the details (it was related to conditional logging features in the bwe code).
lgtm for this, since the alternatives are large undertakings that can't be ready quickly enough
The CQ bit was checked by eladalon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org Link to the patchset: https://codereview.webrtc.org/2957753002/#ps40001 (title: ".")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498657444863310,
"parent_rev": "3d0e7bb90710bdeacccf1611967621e4d6e55f26", "commit_rev":
"d6e9466e7ef8736c5dd4c736c9732506b1b70c1e"}
Message was sent while issue was closed.
Description was changed from ========== No compliation-flag-dependent members in CriticalSection Having members in a class which only exist when certain compliation flags are turned on (unless relating to the target platform) means that those flags must be the same when compiling the module as when including its headers from other modules. This means that code outside of WebRTC runs the risk of misjudging the size of an rtc::CriticalSection, or any class which has an rtc::CriticalSection as a member. (This rule is applied recursively.) If a mismatch occurs, memory corruption is likely. Having discussed this a bit, we have decided that the simplest solution is probably the best - always define those members, even when compilation flags (namely, CS_DEBUG_CHECKS) render it unused. BUG=webrtc:7867 ========== to ========== No compliation-flag-dependent members in CriticalSection Having members in a class which only exist when certain compliation flags are turned on (unless relating to the target platform) means that those flags must be the same when compiling the module as when including its headers from other modules. This means that code outside of WebRTC runs the risk of misjudging the size of an rtc::CriticalSection, or any class which has an rtc::CriticalSection as a member. (This rule is applied recursively.) If a mismatch occurs, memory corruption is likely. Having discussed this a bit, we have decided that the simplest solution is probably the best - always define those members, even when compilation flags (namely, CS_DEBUG_CHECKS) render it unused. BUG=webrtc:7867 Review-Url: https://codereview.webrtc.org/2957753002 Cr-Commit-Position: refs/heads/master@{#18811} Committed: https://chromium.googlesource.com/external/webrtc/+/d6e9466e7ef8736c5dd4c736c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/d6e9466e7ef8736c5dd4c736c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
