In form, inspired by ANDROID_BASE_UNIQUE_FD_DISABLE_IMPLICIT_CONVERSION.
We get occasional bugs about sp double-ownership. When this flag is
enabled, we have:
- you must construct RefBase objects using sp<>::make
- you must construct wp<> objects by converting them to sp<>
- if you want to convert a raw pointer to an sp<> object (this is
possible since the refcount is used internally, and is used commonly
on this*), then you must use 'assertStrongRefExists' semantics which
aborts if there is no strong ref held. That is, if a client uses
std::make_shared and then calls a function which internally used to
call `sp<T>(this)`, you would now call
`sp<T>::assertStrongRefExists(this)`, and the double ownership
problem would become a runtime error.
Bug: 184190315
Test: libutils_test
Change-Id: Ie18d3146420df1808e3733027070ec234dda4e9d
This requires a few other changes to support building libutils
properly. It does appear the windows versions of libutils is
referencing CallStack code, but it doesn't seem to cause any problems.
However, I removed those references completely for the windows build.
Also removed a few extra spaces that seem to have accumulated in the
RefBase.cpp.
Bug: 170465278
Test: Builds and libutils unit tests pass.
Change-Id: Ibeee7791b13636b34bdd592c5420fd91620f752a
Add FALLTHROUGH_INTENDED for clang compiler.
Bug: 112564944
Test: build with global -Wimplicit-fallthrough.
Change-Id: I40f8bbf94e207c9dd90921e9b762ba51abab5777
This reverts commit b9d0753d2b.
Reason for revert: Re-land with MacOS workaround.
Test: Build (on Linux) and boot AOSP, with weak symbols enabled and disabled.
Change-Id: I5150cd90367178f3b039761dca3bccc9c2987df1
This reverts commit 9d3146af22.
Reason for revert: It appears that weak symbols don't work as expected on MacOS, breaking the MacOS aapt build.
Change-Id: Ica0955106485a7bf2e2c3f09ff7910e230eb4139
Move towards crashing if a normally configured RefBase object is
destroyed without ever incrementing the reference count. We've been
threatening to do this for a long time. The previously last known
violation had been fixed.
This also fixes stack trace printing from RefBase, which had previously
been broken, and which we found necessary to track down further
violations of this rule.
Unfortunately, we found several more violations with the aid of
that fix. After existing CLs are submitted, there are
still some failures, but they are no longer numerous. Thus this CL
doesn't actually crash in the event of a violation, but does log a
verbose stack trace if it encounters one.
Bugs have been filed against the remaining known RefBase client offenders.
We plan to enable crashing on usage violations once those are fixed.
The fix for the stack trace printing breakage unfortunately requires
the use of weak symbols in order to avoid a circular build dependency.
We expect to eventually replace this with execinfo.h functionality.
Some random reformatting, driven by consistency with current formatting
requirements.
Add missing include to BacktraceMap.h.
Bug: 79112958
Bug: 30292291
Test: Boot AOSP, Master
Change-Id: I8151c54560c3b6f75ffc4c48229f0388a2066958
The analyzer is known to be very conservative in the face of atomic
operations (e.g. https://bugs.llvm.org/show_bug.cgi?id=34365); this case
is no different.
It's concerned that `delete this;` might read a different value for
`flags`, and proceed to delete `refs`. Since there are many comments
explaining why this won't happen (and it all looks sane to me), use a
NOLINT to silence this warning.
Analyzer warning:
system/core/libutils/RefBase.cpp:445:5: warning: Use of memory after it
is freed
Bug: 27101951
Test: mma. Use-after-free warning is gone.
Change-Id: Ic1623971bd1bad546fbb12a79439116c89a6762d
moved Foo.h as first include of Foo.cpp, and
removed redundant includes.
Made NativeHandle non virtual.
Test: run & compile
Bug: n/a
Change-Id: I37fa746cd42c9ba23aba181f84cb6c619386406a
The warnings in these files were hidden by -isystem
framework/native/include.
Bug: 31752268
Test: m -j
Change-Id: I2a54376aea380ee24e6483fb7d35fdfe8991c490
system/core/include is included in the global include path using
-isystem, which hides all warnings. Fix warnings in libutils
headers in preparation for moving from -isystem to -I.
- Fix implicit cast from int64_t to long in Condition.h. Remove
the __LP64__ check and always compare against LONG_MAX before
casting.
- Fix implicit cast from size_t to ssize_t in KeyedVector.h
- Fix -Wshadow-field-in-constructor warnings in Looper.h and RefBase.h
- Move destructors for MessageHandler and LooperCallback to Looper.cpp
and ReferenceRenamer and VirtualLightRefBase to RefBase.cpp to prevent
vtables in every compilation unit.
- Declare template variables in Singleton.h
- Fix old-style casts in StrongPointer.h and TypeHelpers.h
- Use template metaprogramming in TypeHelpers.h to avoid warnings on
memmove on non-trivial types.
- Add an assignment operator to key_value_pair_t to complete
rule-of-three
- Use memcpy instead of dereferencing a reinterpret_casted pointer to
treat the bits of a float or double as int32_t or int64_t
- Escape unicode sequences inside doxygen comments between \code and
\endcode
- Remove WIN32 ZD definition in Compat.h, %zd works fine with mingw
- Fix WIN32 printf warnings in Filemap.cpp
- Initialize mNullValue with 0 in LruCache.h, some of the tests use a
non-pointer type for TValue.
Test: m -j native
Bug: 31492149
Change-Id: I385a05a3ca01258e44fe3b37ef77e4aaff547b26
This prevents two different kinds of client errors from causing
undetected memory corruption, and helps with the detection of others:
1. We no longer deallocate objects when the weak count goes to zero
and there have been no strong references. This otherwise causes
us to return a garbage object from a constructor if the constructor
allocates and deallocates a weak pointer to this. And we do know
that clients allocate such weak pointers in constructors and their
lifetime is hard to trace.
2. We abort if a RefBase object is explicitly destroyed while
the weak count is nonzero. Otherwise a subsequent decrement
would cause a write to potentially reallocated memory.
3. We check counter values returned by atomic decrements for
plausibility, and fail immediately if they are not plausible.
We unconditionally log any cases in which 1 changes behavior
from before. We abort in cases in which 2 changes behavior, since
those reflect clear bugs.
In case 1, a log message now indicates a possible leak. We have
not seen such a message in practice.
The third point introduces a small amount of overhead into the
reference count decrement path. But this should be negligible
compared to the actual decrement cost.
Add a test for promote/attemptIncStrong that tries to check for
both (1) above and concurrent operation of attemptIncStrong.
Add some additional warnings and explanations to the RefBase
documentation.
Bug: 30503444
Bug: 30292291
Bug: 30292538
Change-Id: Ida92b9a2e247f543a948a75d221fbc0038dea66c
Add basic interface documentation to RefBase.h.
Much, but not all, of this is cut-and-pasted from an email message
from Mathias Agopian. The rest is reconstructed from the code.
Delete some, now redundant, text from Refbase.cpp, and add a bit
more about the implementation strategy.
Some minor fixes to internal comments.
Bug: 30292291
Change-Id: I56518ae5553bc6de0cc2331778e7fcf2e6c4fd87
The compensating onLastStrongRef call could be made even when there
was no onIncStrongAttempted call to compensate for. This
happened in the OBJECT_LIFETIME_STRONG case when e.g. curCount
was initially zero, but was concurrently incremented by another
thread.
I believe the old code was also incorrect in the
curCount = INITIAL_STRONG_VALUE + 1 case,
which seems to be possible under unlikely conditions.
In that case, I believe the compensating call IS needed.
Thus the condition was also changed.
Bug: 30503444
Change-Id: I44bcbcbb1264e4b52b6d3750dc39b041c4140381
Convert to use std::atomic directly.
Consistently use relaxed ordering for increments, release ordering
for decrements, and an added acquire fence when the count goes to
zero.
Fix what looks like another race in attemptIncStrong:
It seems entirely possible that the final adjustment for
INITIAL_STRONG_VALUE would see e.g. INITIAL_STRONG_VALUE + 1,
since we could be running in the middle of another initial
increment.
Attempt to somewhat document what this actually does, and
what's expected from the client. Hide the documentation in
the .cpp file for now.
Remove a confusing redundant test in decWeak. OBJECT_LIFETIME_STRONG
and OBJECT_LIFETIME_WEAK are the only options, in spite of some
of the original comments.
It's conceivable that either of these issues has resulted in
actual crashes, though I would guess the probability is small.
It's hard enough to reason about this code without the bugs.
Bug: 28705989
Change-Id: I4107a56c3fc0fdb7ee17fc8a8f0dd7fb128af9d8
Also cleans up two instances of open() with useless mode params, and
changes a few uses of snprintf to use sizeof(buffer) instead of
hardcoded buffer sizes.
Change-Id: If11591003d910c995e72ad8f75afd072c255a3c5
- Deal with some -Wunused issues
- Override PRI macros (windows)
- Revert use of PRI macros on off64_t (linux)
- Deal with a gnu++11 complaince issue
Change-Id: Ie66751293bd84477a5a6dfd8a57e700a16e36964
This was broken about 5 months ago in change I78435ed49aa196a0efb45bf9b2d58b62c41737d3.
See: https://goto.google.com/jhtss
Change-Id: Icc32993552efed3015bc1b79a7bd872d7510e020
