Hopefully we can get away with always storing this by finding a nice way to compress this information. This shouldn't be too difficult. After thinking about the uses for this mapping, we should only need to consult this mapping for the top frame in a stack trace. All other parent frames should have the call site index in an accurate state on the stack which means we'll just be able to consult that CallSiteIndex to retrieve the CodeOrigin. The top frame, though, might not have the CallSiteIndex be in an accurate state. This could happen for a number of reasons, for example: let's say we make a call from inside an inlined function, but then pause the process after that call took place, and before any other calls are made, and the PC is now at code that isn't from the same inlined function. The CallSiteIndex will still show us executing in the inlined function, which isn't correct. There are also parts of the function, in the prologue for example, where we could just have garbage bits for the CallSiteIndex. Having a PC=>CodeOrigin map would solve these problems. I'm sure there are more situations it will help with, too. There will probably be times, though, where the PC=>CodeOrigin map won't help us, but we're still able to take a valid stack trace, and in those situations, we will probably have to forfeit getting high fidelity location information and settle for just knowing which function we're executing inside.
Created attachment 269310 [details] WIP it's a start.
Created attachment 269522 [details] WIP
Created attachment 269532 [details] WIP now accounting for inlined frames at the top of the stack in a stack traces.
Created attachment 269814 [details] WIP I think it's almost done.
Created attachment 269839 [details] more cleanup
Created attachment 269843 [details] more cleanup
Created attachment 269911 [details] make findPC find more PCs
Created attachment 269913 [details] patch
Attachment 269913 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.h:42: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:331: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:344: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/B3PCToOriginMap.h:33: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/JavaScriptCore/b3/B3PCToOriginMap.h:61: Missing space before { [whitespace/braces] [5] ERROR: Source/JavaScriptCore/jit/JIT.h:50: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:54: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:178: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:27: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:126: Missing space before { [whitespace/braces] [5] ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:155: Missing space before { [whitespace/braces] [5] ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:159: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:175: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:267: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 14 in 43 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 269921 [details] patch hopefully fix builds. fix some style issues
Created attachment 269924 [details] patch rebased
Attachment 269924 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:331: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:344: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/B3PCToOriginMap.h:61: Missing space before { [whitespace/braces] [5] ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:176: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:126: Missing space before { [whitespace/braces] [5] ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:155: Missing space before { [whitespace/braces] [5] ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:159: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:175: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:267: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 9 in 46 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 269924 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=269924&action=review > Source/JavaScriptCore/inspector/JSGlobalObjectScriptDebugServer.cpp:46 > + m_globalObject.vm().setShouldBuildPCToCodeOriginMapping(); I'm moving this into Debugger::attach().
Comment on attachment 269924 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=269924&action=review > Source/JavaScriptCore/b3/B3PCToOriginMap.h:58 > + if (m_ranges.last().origin == origin || !origin) > + return; this should be removed and is locally
Comment on attachment 269924 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=269924&action=review > Source/JavaScriptCore/bytecode/CodeBlock.cpp:4290 > +#if ENABLE(JIT) I'm going to make PCToCodeOrigin map only build under ENABLE(JIT)
Created attachment 269947 [details] patch
Attachment 269947 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:331: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:344: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/B3PCToOriginMap.h:59: Missing space before { [whitespace/braces] [5] ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:176: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:128: Missing space before { [whitespace/braces] [5] ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:157: Missing space before { [whitespace/braces] [5] ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:161: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:177: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:269: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 9 in 46 files If any of these errors are false positives, please file a bug against check-webkit-style.
I haven't looked at the patch fully but what about adding an option that always turns this on? I think it might be useful for debugging. Obviously, the option can come in a later patch.
(In reply to comment #18) > I haven't looked at the patch fully but what about adding an option that > always turns this on? I think it might be useful for debugging. Obviously, > the option can come in a later patch. Yeah sure I'll add one. It'd be even better if that option created a global table so you could always go from: (VM, PC) => CodeBlock.
Comment on attachment 269947 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=269947&action=review > Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:81 > +class DeltaCompressionBuilder { > +public: > + DeltaCompressionBuilder(size_t maxSize) > + : m_offset(0) > + , m_maxSize(maxSize) > + { > + m_buffer = static_cast<uint8_t*>(fastMalloc(m_maxSize)); > + } > + > + template <typename T> > + void write(T item) > + { > + RELEASE_ASSERT(m_offset + sizeof(T) <= m_maxSize); > + *bitwise_cast<T*>(m_buffer + m_offset) = item; > + m_offset += sizeof(T); > + } > + > + uint8_t* m_buffer; > + size_t m_offset; > + size_t m_maxSize; > +}; > + > +class DeltaCompresseionReader { > +public: > + DeltaCompresseionReader(uint8_t* buffer, size_t size) > + : m_buffer(buffer) > + , m_size(size) > + , m_offset(0) > + { } > + > + template <typename T> > + T read() > + { > + RELEASE_ASSERT(m_offset + sizeof(T) <= m_size); > + T result = *bitwise_cast<T*>(m_buffer + m_offset); > + m_offset += sizeof(T); > + return result; > + } > + > +private: > + uint8_t* m_buffer; > + size_t m_size; > + size_t m_offset; After speaking w/ Andreas today, I realize that this probably won't play nice w/ ARM alignment requirements. I'm going to make the necessary changes to make this work.
Created attachment 270246 [details] patch
Created attachment 270253 [details] patch rebased
Attachment 270253 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:331: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:344: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/B3PCToOriginMap.h:59: Missing space before { [whitespace/braces] [5] ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:176: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:138: Missing space before { [whitespace/braces] [5] ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:167: Missing space before { [whitespace/braces] [5] ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:171: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:187: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/PCToCodeOriginMap.cpp:279: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 9 in 47 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 270253 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=270253&action=review > Source/JavaScriptCore/b3/air/AirGenerate.cpp:239 > + pcToOriginMap.appendItem(jit.label(), Origin()); Can you add a FIXME and file a bug about making late paths have an origin?
landed in: http://trac.webkit.org/changeset/195865