[Bug libstdc++/54351] New: ~unique_ptr() should not set __p to null
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54351
Bug #: 54351
Summary: ~unique_ptr() should not set __p to null
Classification: Unclassified
Product: gcc
Version: 4.7.1
Status: UNCONFIRMED
Severity: normal
Priority: P3
Component: libstdc++
AssignedTo: unassig...@gcc.gnu.org
ReportedBy: gro...@google.com
libstdc++'s unique_ptr destructor is currently implemented as:
~unique_ptr() noexcept { reset(); }
This has the effect of resetting the stored pointer to null, and then invoking
the deleter on the formerly-stored pointer. I believe this is inconsistent with
the language standard, which specifies the destructor as "If get() == nullptr
there are no effects. Otherwise get_deleter()(get())." (note no mention of any
side effects on the value of the stored pointer). This is a problem because
this implementation will break code that (legitimately, AFAICT) relies on being
able to continue to invoke operations on the scoped_ptr while the destructor is
executing.
The fix is to reimplement the destructor (in both the base template and the
array specialization) as
~unique_ptr() noexcept {
if (__p != pointer())
get_deleter()(__p);
}
If the intent is to zero out __p to help catch use-after-destruction errors, I
believe it would be permissible to set __p to null after the call to
get_deleter(), because at that point the change would no longer be visible to
conforming code.
To make this concrete, here's an example: the following program prints "bad"
under libstdc++, but I believe a standard-conforming implementation is required
to print "good":
=
#include
#include
using std::cout;
using std::endl;
using std::unique_ptr;
struct A;
struct B {
unique_ptr a;
};
struct A {
B* b;
~A() {
if (b->a == nullptr) {
cout << "bad" << endl;
} else {
cout << "good" << endl;
}
}
};
int main(int argc, char** argv) {
B b;
b.a.reset(new A);
b.a->b = &b;
}
===
As a point of comparison, MSVC++ 2010 prints "good" on this example program.
[Bug libstdc++/54351] ~unique_ptr() should not set __p to null
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54351 --- Comment #5 from Geoff Romer 2012-08-22 19:11:17 UTC --- Don't forget the array specialization. Doesn't the first line of your new destructor shadow the __p member with a __p local variable? Why is that line needed at all?
[Bug libstdc++/54351] ~unique_ptr() should not set stored pointer to null
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54351 --- Comment #7 from Geoff Romer 2012-08-22 19:49:28 UTC --- (In reply to comment #6) > (In reply to comment #5) > > Don't forget the array specialization. > > I won't :-) > > > Doesn't the first line of your new destructor shadow the __p member with a > > __p > > local variable? Why is that line needed at all? > > There is no __p member. Ah, sorry, I was evidently misreading something. I've corrected the bug description.
[Bug libstdc++/58159] New: unique_ptr::reset should have debug assertion for "self-reset"
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58159 Bug ID: 58159 Summary: unique_ptr::reset should have debug assertion for "self-reset" Product: gcc Version: unknown Status: UNCONFIRMED Severity: normal Priority: P3 Component: libstdc++ Assignee: unassigned at gcc dot gnu.org Reporter: gromer at google dot com unique_ptr::reset() should have a debug assertion for the case that the input pointer is not null, and is equal to the pointer already stored. This is virtually always an error: it violates the basic ownership logic of unique_ptr, and leaves unique_ptr holding a dangling pointer. Strictly speaking, this is not undefined behavior in itself, but almost any subsequent operation other than release() (even destroying the unique_ptr) will produce undefined behavior, so this seems like a highly defensible condition to assert on, at least in debug mode. This kind of error is rare, but it does happen; see e.g. https://code.google.com/p/chromium/issues/detail?id=162971.
[Bug libstdc++/58159] unique_ptr::reset should have debug assertion for "self-reset"
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58159 Geoff Romer changed: What|Removed |Added CC||gromer at google dot com Severity|normal |enhancement
[Bug libstdc++/58159] unique_ptr::reset should have debug assertion for "self-reset"
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58159 --- Comment #3 from Geoff Romer --- What's the standard of review here? If we can only assert on undefined behavior, even in debug mode, then this just can't be done (although maybe we should make this undefined in the Standard). If we can assert on behavior that's technically well-defined, but very likely to lead to undefined behavior, and very far from accepted usage norms, then I think this is OK (even for custom deleters). The kinds of uses you're concerned about (e.g. using p.reset(p.get()) to invoke some operation on *p) seem really marginal to me.
[Bug preprocessor/20262] __LINE__ implementation flaky.
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=20262 Geoff Romer changed: What|Removed |Added CC||gromer at google dot com --- Comment #5 from Geoff Romer --- The issue isn't the flexibility in "presumed", it's the flexibility in "source line": the standard defines line number in terms of the "current token", but gives no explicit guidance about which token is considered "current" in a case like this, where the preprocessor is arguably expanding two different macros at the same time.
[Bug libstdc++/54351] ~unique_ptr() should not set stored pointer to null
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54351 --- Comment #11 from Geoff Romer 2012-12-18 21:59:23 UTC --- >From discussion on the C++ LWG reflector, it appears that the standard's requirements on library types are intended to apply only during their lifetime, although the standard does not currently make this clear. LWG 2224 (http://cplusplus.github.com/LWG/lwg-active.html#2224) is tracking this issue, and its resolution should give a definitive answer. That being the case, the old behavior was not a bug, but conformant with the the intent of the standard (if not the precise wording). The new behavior is conformant as well, of course, so it's up to you whether to revert the changes; I just wanted to document for future reference that ~unique_ptr is in fact permitted to modify the stored pointer before invoking the deleter, so this bug report should not be an obstacle to e.g. having ~unique_ptr store a poison value in order to catch client bugs.
[Bug libstdc++/60970] New: Support std::hash with enum types (LWG 2148)
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60970 Bug ID: 60970 Summary: Support std::hash with enum types (LWG 2148) Product: gcc Version: unknown Status: UNCONFIRMED Severity: normal Priority: P3 Component: libstdc++ Assignee: unassigned at gcc dot gnu.org Reporter: gromer at google dot com std::hash appears to be undefined when T is an enum type, contrary to the resolution of LWG 2148 (http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#2148).
[Bug libstdc++/38265] STL treats explicit constructors as converting constructors
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38265 Geoff Romer changed: What|Removed |Added CC||gromer at google dot com --- Comment #14 from Geoff Romer --- > The C++11 standard clarifies that this is intended to work. The range > constructors require the type to be EmplaceConstructible into the container > from the iterator's value_type But see [sequence.reqmts]/p3: "i and j denote iterators satisfying input iterator requirements and refer to elements implicitly convertible to value_type". So this is not "intended to work" per se; a conforming library could disallow it. However, the notes on LWG 536 say "Some support, not universal, for respecting the explicit qualifier", so it looks like the standard intentionally permits this as a conforming extension. In principle, I think "perfect initialization" is what's called for here: the range ctor should be explicit if and only if it uses an explicit constructor for value_type. However, I doubt it's worth the trouble (OTOH, it definitely will be worth the trouble when/if we get single-argument range constructors).
[Bug libstdc++/58159] unique_ptr::reset should have debug assertion for "self-reset"
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58159 --- Comment #6 from Geoff Romer --- A Chromium maintainer privately pointed out a use case that would be thwarted by a check like this: basically, unique_ptr is used to hold pointers from a legacy API, using a custom deleter that decrements a reference count rather than actually destroying the object. If the legacy API returns the same pointer from multiple calls, this can lead to reset() being legitimately called with a pointer equal to the stored value. So on reflection, I agree this can only be done for default_delete.
