Bug 217588 – CallFrame.h:324:38: warning: calling ‘void* __builtin_frame_address(unsigned int)’ with a nonzero argument is unsafe [-Wframe-address]

• Status: RESOLVED
• Resolution: WORKSFORME
• Component: JavaScriptCore

[Michael Catanzaro]

I've never seen -Wframe-address before. Anyway, we have this code in CallFrame.h:

#if USE(BUILTIN_FRAME_ADDRESS)
#define DECLARE_CALL_FRAME(vm) \
    ({ \
        ASSERT(JSC::isFromJSCode(removeCodePtrTag<void*>(__builtin_return_address(0)))); \
        bitwise_cast<JSC::CallFrame*>(__builtin_frame_address(1)); \
    })
#else
#define DECLARE_CALL_FRAME(vm) ((vm).topCallFrame)
#endif

This code was added in r251475, nearly a year ago. I'm not sure why it hasn't caused warnings for me before now. (I'm using GCC 10.2.1 20200826 currently. Maybe the warning was added in a GCC point release? I've been using GCC 10 all year.) Anyway, the manpage says:

       -Wframe-address
           Warn when the __builtin_frame_address or __builtin_return_address is called with an argument greater
           than 0.  Such calls may return indeterminate values or crash the program.  The warning is included in
           -Wall.

Documentation is here: https://gcc.gnu.org/onlinedocs/gcc/Return-Address.html. "Calling this function with a nonzero argument can have unpredictable effects, including crashing the calling program. As a result, calls that are considered unsafe are diagnosed when the -Wframe-address option is in effect. Such calls should only be made in debugging situations."

So... that seems bad. This seems like a question for compiler people, then: should we disable USE_BUILTIN_FRAME_ADDRESS, or suppress the warning and hope for the best...?

[570/3175] Building CXX object Source/WebCore/CMakeFiles/...es/WebCore/unified-sources/UnifiedSource-48793971-1.cpp.o
In file included from DerivedSources/ForwardingHeaders/JavaScriptCore/ArgList.h:24,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/JSArray.h:23,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/ArrayAllocationProfile.h:29,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/JSGlobalObject.h:24,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/InternalFunctionAllocationProfile.h:28,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/FunctionRareData.h:28,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/JSFunction.h:26,
                 from DerivedSources/WebCore/JSDOMBindingInternalsBuiltins.h:35,
                 from DerivedSources/WebCore/WebCoreJSBuiltinInternals.h:38,
                 from ../../Source/WebCore/bindings/js/JSDOMGlobalObject.h:29,
                 from ../../Source/WebCore/bindings/js/JSDOMWrapper.h:24,
                 from ../../Source/WebCore/domjit/DOMJITHelpers.h:29,
                 from ../../Source/WebCore/domjit/DOMJITHelpers.cpp:27,
                 from DerivedSources/WebCore/unified-sources/UnifiedSource-48793971-1.cpp:1:
../../Source/WebCore/domjit/JSDocumentDOMJIT.cpp: In function ‘JSC::EncodedJSValue WebCore::DOMJIT::operationToJSElement(JSC::JSGlobalObject*, void*)’:
DerivedSources/ForwardingHeaders/JavaScriptCore/CallFrame.h:324:38: warning: calling ‘void* __builtin_frame_address(unsigned int)’ with a nonzero argument is unsafe [-Wframe-address]
  324 |         bitwise_cast<JSC::CallFrame*>(__builtin_frame_address(1)); \
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Source/WebCore/domjit/JSDocumentDOMJIT.cpp:152:33: note: in expansion of macro ‘DECLARE_CALL_FRAME’
  152 |     JSC::CallFrame* callFrame = DECLARE_CALL_FRAME(vm);
      |                                 ^~~~~~~~~~~~~~~~~~
../../Source/WebCore/domjit/JSDocumentDOMJIT.cpp: In function ‘JSC::EncodedJSValue WebCore::DOMJIT::operationToJSHTMLElement(JSC::JSGlobalObject*, void*)’:
DerivedSources/ForwardingHeaders/JavaScriptCore/CallFrame.h:324:38: warning: calling ‘void* __builtin_frame_address(unsigned int)’ with a nonzero argument is unsafe [-Wframe-address]
  324 |         bitwise_cast<JSC::CallFrame*>(__builtin_frame_address(1)); \
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Source/WebCore/domjit/JSDocumentDOMJIT.cpp:162:33: note: in expansion of macro ‘DECLARE_CALL_FRAME’
  162 |     JSC::CallFrame* callFrame = DECLARE_CALL_FRAME(vm);
      |                                 ^~~~~~~~~~~~~~~~~~
../../Source/WebCore/domjit/JSDocumentDOMJIT.cpp: In function ‘JSC::EncodedJSValue WebCore::DOMJIT::operationToJSDocument(JSC::JSGlobalObject*, void*)’:
DerivedSources/ForwardingHeaders/JavaScriptCore/CallFrame.h:324:38: warning: calling ‘void* __builtin_frame_address(unsigned int)’ with a nonzero argument is unsafe [-Wframe-address]
  324 |         bitwise_cast<JSC::CallFrame*>(__builtin_frame_address(1)); \
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Source/WebCore/domjit/JSDocumentDOMJIT.cpp:172:33: note: in expansion of macro ‘DECLARE_CALL_FRAME’
  172 |     JSC::CallFrame* callFrame = DECLARE_CALL_FRAME(vm);
      |                                 ^~~~~~~~~~~~~~~~~~
../../Source/WebCore/domjit/JSDocumentDOMJIT.cpp: In function ‘JSC::EncodedJSValue WebCore::DOMJIT::operationToJSNode(JSC::JSGlobalObject*, void*)’:
DerivedSources/ForwardingHeaders/JavaScriptCore/CallFrame.h:324:38: warning: calling ‘void* __builtin_frame_address(unsigned int)’ with a nonzero argument is unsafe [-Wframe-address]
  324 |         bitwise_cast<JSC::CallFrame*>(__builtin_frame_address(1)); \
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Source/WebCore/domjit/JSDocumentDOMJIT.cpp:182:33: note: in expansion of macro ‘DECLARE_CALL_FRAME’
  182 |     JSC::CallFrame* callFrame = DECLARE_CALL_FRAME(vm);
      |                                 ^~~~~~~~~~~~~~~~~~
../../Source/WebCore/domjit/JSDocumentDOMJIT.cpp: In function ‘JSC::EncodedJSValue WebCore::DOMJIT::operationToJSContainerNode(JSC::JSGlobalObject*, void*)’:
DerivedSources/ForwardingHeaders/JavaScriptCore/CallFrame.h:324:38: warning: calling ‘void* __builtin_frame_address(unsigned int)’ with a nonzero argument is unsafe [-Wframe-address]
  324 |         bitwise_cast<JSC::CallFrame*>(__builtin_frame_address(1)); \
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Source/WebCore/domjit/JSDocumentDOMJIT.cpp:192:33: note: in expansion of macro ‘DECLARE_CALL_FRAME’
  192 |     JSC::CallFrame* callFrame = DECLARE_CALL_FRAME(vm);
      |                                 ^~~~~~~~~~~~~~~~~~

[Michael Catanzaro]

Anyone have an opinion on what to do here? (Especially Yusuke, since it's your code?)

We could always silence the warning... though that doesn't sound great, unless the code is only invoked immediately before a crash. And it looks like that's not the case.

[Radar WebKit Bug Importer]

<rdar://problem/70421807>

[Michael Catanzaro]

I see these warnings are suppressed by r268661 (thanks Joonghun). I would also add in calls to IGNORE_WARNINGS_END() as well to restore the warnings stack to its original state, though that's not needed.

Not closing this bug, though, because I wonder if JSC::JITOperationPrologueCallFrameTracer is actually safe to use in production code. I'm suspecting not. Yusuke, do you agree?

[Joonghun Park]

(In reply to Michael Catanzaro from comment #3)
> I see these warnings are suppressed by r268661 (thanks Joonghun). I would
> also add in calls to IGNORE_WARNINGS_END() as well to restore the warnings
> stack to its original state, though that's not needed.
> 
> Not closing this bug, though, because I wonder if
> JSC::JITOperationPrologueCallFrameTracer is actually safe to use in
> production code. I'm suspecting not. Yusuke, do you agree?

Hi, Michael.
FYI, I added IGNORE_WARNINGS_END macro call in https://trac.webkit.org/changeset/268663/webkit :)

[Darin Adler]

I think this is a performance optimization. We can turn it off for any platforms where we don’t need the optimization. The question is whether it works in practice. Scary comments from the compiler documentation don’t fully answer that question.

[Michael Catanzaro]

I guess we'd have received bug reports before now if it wasn't working in practice.