Issue metadata
Sign in to add a comment
|
TraceBufferChunk::EstimateTraceMemoryOverhead may use incorrect value due to string literal hashing. |
||||||||||||||||||||||||
Issue descriptionChrome Version: 60.0.3081.0 (Developer Build) (64-bit) What steps will reproduce the problem? N/A. It might be impossible to produce the problem on current C compilers... What happens instead? C++ does not guarantee that different string literals with the same contents evaluate to the same pointer. std::hash<char*> hashes the pointer, not the string contents. This means that the unordered_map at [1] is dangerous to use correctly. The programmer needs to ensure that they only use one string literal per category or seemingly identical strings may refer to different categories. Here [2] and [3] may not refer to the same unordered_map key if their string literals evaluate to different pointers. This is allowed to happen according to the C++ standard. This could cause num_cached_estimated_events to incorrectly be zero. [1] https://cs.chromium.org/chromium/src/base/trace_event/trace_event_memory_overhead.h?rcl=b9c3c20ba9a2a3c05b6d0f976e4a2d71c8a74fa6&l=66 [2] https://cs.chromium.org/chromium/src/base/trace_event/trace_event_impl.cc?rcl=da1b049fa7cad45f62965d461c5119a48775ab38&l=201 [3] https://cs.chromium.org/chromium/src/base/trace_event/trace_buffer.cc?rcl=da1b049fa7cad45f62965d461c5119a48775ab38&l=276 Since I can't actually produce any issues from this, it's possibly not super high priority. But it would be nice we guaranteed that string literals were never hashed against other identical string literals. Context: https://bugs.chromium.org/p/chromium/issues/detail?id=495628#c10
,
May 1 2017
,
May 1 2017
I suggest passing a custom hash and equality to the unordered_map, which would compare the values of the strings, rather than their addresses.
,
May 2 2017
Yup this is a bug, we do rely on pointer equality -> string equality, but should not rely on pointer inequality -> string inequality, which is what is happening here. Don't want to do any hashing here for performance reasons. My plan here is to get rid of that map all together and make this an enum.
,
May 2 2017
Fix coming in https://codereview.chromium.org/2857543002/
,
May 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eb6696a1bfb1c32c6d2d1baf9a0dabcc632119e7 commit eb6696a1bfb1c32c6d2d1baf9a0dabcc632119e7 Author: primiano <primiano@chromium.org> Date: Tue May 02 13:01:14 2017 tracing: Simplify TraceEventMemoryOverhead, use an enum insted of a map TraceEventMemoryOverhead is a rather simple class which only purpose is accounting various sources of memory overhead due to tracing itself. For debugging purposes it keeps track of the actual class causing overhead, so that they can be inspected distinctly in the tracing UI The current implementation was using a small_map indexed by const char* pointer, which has a bunch of problems: - base::small_map can degenerate into a STL map when exceeding the inline capacity, which in turn can cause some extra impredictability, see "[chromium-dev] Please avoid std::unordered_map". - The map implicitly assumes that two identical string literals will have identical pointers, which is not true, esepcially in component builds. - Any sort of map is a really over-engineered solution, given that the sources of overheads we care about are known a priori and are a manageable number. Hence, this CL is switching the implementation of TraceEventMemoryOverhead to be just enum-base, which ditches completely the map. BUG= 717223 Review-Url: https://codereview.chromium.org/2857543002 Cr-Commit-Position: refs/heads/master@{#468611} [modify] https://crrev.com/eb6696a1bfb1c32c6d2d1baf9a0dabcc632119e7/base/trace_event/heap_profiler_allocation_register.cc [modify] https://crrev.com/eb6696a1bfb1c32c6d2d1baf9a0dabcc632119e7/base/trace_event/heap_profiler_stack_frame_deduplicator.cc [modify] https://crrev.com/eb6696a1bfb1c32c6d2d1baf9a0dabcc632119e7/base/trace_event/heap_profiler_type_name_deduplicator.cc [modify] https://crrev.com/eb6696a1bfb1c32c6d2d1baf9a0dabcc632119e7/base/trace_event/trace_buffer.cc [modify] https://crrev.com/eb6696a1bfb1c32c6d2d1baf9a0dabcc632119e7/base/trace_event/trace_event_argument.cc [modify] https://crrev.com/eb6696a1bfb1c32c6d2d1baf9a0dabcc632119e7/base/trace_event/trace_event_impl.cc [modify] https://crrev.com/eb6696a1bfb1c32c6d2d1baf9a0dabcc632119e7/base/trace_event/trace_event_memory_overhead.cc [modify] https://crrev.com/eb6696a1bfb1c32c6d2d1baf9a0dabcc632119e7/base/trace_event/trace_event_memory_overhead.h [modify] https://crrev.com/eb6696a1bfb1c32c6d2d1baf9a0dabcc632119e7/base/trace_event/trace_log.cc
,
May 3 2017
|
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by zoeclifford@chromium.org
, May 1 2017