Closed Bug 1356709 Opened 3 years ago Closed 3 years ago

s/src/jit/arm64/vixl/Debugger-vixl.cpp:1123:44: error: ignoring return value of 'bool mozilla::Vector<T, N, AllocPolicy>::append(U&&) [with U = vixl::Token*; T = vixl::Token*; long unsigned int MinInlineCapacity = 0ul; AllocPolicy = js::SystemAllocPolicy]

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: glandium, Assigned: sfink)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

MOZ_AUTOMATION is supposed to be set on all builds, but currently isn't. When set, --enable-warnings-as-errors is enabled by default.

When that happens, SM arm64 debug builds are busted with:

[task 2017-04-14T22:43:47.839283Z] In file included from /home/worker/workspace/build/src/obj-spider/js/src/Unified_cpp_js_src22.cpp:20:0:
[task 2017-04-14T22:43:47.839363Z] /home/worker/workspace/build/src/js/src/jit/arm64/vixl/Debugger-vixl.cpp: In static member function 'static vixl::DebugCommand* vixl::DebugCommand::Parse(char*)':
[task 2017-04-14T22:43:47.839436Z] /home/worker/workspace/build/src/js/src/jit/arm64/vixl/Debugger-vixl.cpp:1123:44: error: ignoring return value of 'bool mozilla::Vector<T, N, AllocPolicy>::append(U&&) [with U = vixl::Token*; T = vixl::Token*; long unsigned int MinInlineCapacity = 0ul; AllocPolicy = js::SystemAllocPolicy]', declared with attribute warn_unused_result [-Werror=unused-result]
[task 2017-04-14T22:43:47.839457Z]          args.append(Token::Tokenize(chunk));
[task 2017-04-14T22:43:47.839474Z]                                             ^
[task 2017-04-14T22:43:47.839543Z] /home/worker/workspace/build/src/js/src/jit/arm64/vixl/Debugger-vixl.cpp:1124:28: error: ignoring return value of 'bool mozilla::Vector<T, N, AllocPolicy>::append(U&&) [with U = vixl::Token*&; T = vixl::Token*; long unsigned int MinInlineCapacity = 0ul; AllocPolicy = js::SystemAllocPolicy]', declared with attribute warn_unused_result [-Werror=unused-result]
[task 2017-04-14T22:43:47.839563Z]          args.append(format);
[task 2017-04-14T22:43:47.839575Z]                             ^
[task 2017-04-14T22:43:47.839644Z] /home/worker/workspace/build/src/js/src/jit/arm64/vixl/Debugger-vixl.cpp:1128:44: error: ignoring return value of 'bool mozilla::Vector<T, N, AllocPolicy>::append(U&&) [with U = vixl::Token*; T = vixl::Token*; long unsigned int MinInlineCapacity = 0ul; AllocPolicy = js::SystemAllocPolicy]', declared with attribute warn_unused_result [-Werror=unused-result]
[task 2017-04-14T22:43:47.839665Z]          args.append(Token::Tokenize(chunk));
[task 2017-04-14T22:43:47.839683Z]                                             ^
[task 2017-04-14T22:43:47.839751Z] /home/worker/workspace/build/src/js/src/jit/arm64/vixl/Debugger-vixl.cpp:1131:42: error: ignoring return value of 'bool mozilla::Vector<T, N, AllocPolicy>::append(U&&) [with U = vixl::Token*; T = vixl::Token*; long unsigned int MinInlineCapacity = 0ul; AllocPolicy = js::SystemAllocPolicy]', declared with attribute warn_unused_result [-Werror=unused-result]
[task 2017-04-14T22:43:47.839771Z]        args.append(Token::Tokenize(chunk));
[task 2017-04-14T22:43:47.839786Z]                                           ^
[task 2017-04-14T22:43:47.847611Z] In file included from /home/worker/workspace/build/src/obj-spider/js/src/Unified_cpp_js_src22.cpp:29:0:
[task 2017-04-14T22:43:47.847691Z] /home/worker/workspace/build/src/js/src/jit/arm64/vixl/Decoder-vixl.cpp: In member function 'void vixl::Decoder::AppendVisitor(vixl::DecoderVisitor*)':
[task 2017-04-14T22:43:47.847773Z] /home/worker/workspace/build/src/js/src/jit/arm64/vixl/Decoder-vixl.cpp:115:32: error: ignoring return value of 'bool mozilla::Vector<T, N, AllocPolicy>::append(U&&) [with U = vixl::DecoderVisitor*&; T = vixl::DecoderVisitor*; long unsigned int MinInlineCapacity = 8ul; AllocPolicy = js::SystemAllocPolicy]', declared with attribute warn_unused_result [-Werror=unused-result]
[task 2017-04-14T22:43:47.847798Z]    visitors_.append(new_visitor);
[task 2017-04-14T22:43:47.847827Z]                                 ^
[task 2017-04-14T22:43:47.848905Z] /home/worker/workspace/build/src/js/src/jit/arm64/vixl/Decoder-vixl.cpp: In member function 'void vixl::Decoder::PrependVisitor(vixl::DecoderVisitor*)':
[task 2017-04-14T22:43:47.848992Z] /home/worker/workspace/build/src/js/src/jit/arm64/vixl/Decoder-vixl.cpp:120:51: error: ignoring return value of 'T* mozilla::Vector<T, N, AllocPolicy>::insert(T*, U&&) [with U = vixl::DecoderVisitor*&; T = vixl::DecoderVisitor*; long unsigned int MinInlineCapacity = 8ul; AllocPolicy = js::SystemAllocPolicy]', declared with attribute warn_unused_result [-Werror=unused-result]
[task 2017-04-14T22:43:47.849017Z]    visitors_.insert(visitors_.begin(), new_visitor);
[task 2017-04-14T22:43:47.849037Z]                                                    ^
[task 2017-04-14T22:43:47.849760Z] /home/worker/workspace/build/src/js/src/jit/arm64/vixl/Decoder-vixl.cpp: In member function 'void vixl::Decoder::InsertVisitorBefore(vixl::DecoderVisitor*, vixl::DecoderVisitor*)':
[task 2017-04-14T22:43:47.849848Z] /home/worker/workspace/build/src/js/src/jit/arm64/vixl/Decoder-vixl.cpp:128:40: error: ignoring return value of 'T* mozilla::Vector<T, N, AllocPolicy>::insert(T*, U&&) [with U = vixl::DecoderVisitor*&; T = vixl::DecoderVisitor*; long unsigned int MinInlineCapacity = 8ul; AllocPolicy = js::SystemAllocPolicy]', declared with attribute warn_unused_result [-Werror=unused-result]
[task 2017-04-14T22:43:47.849871Z]        visitors_.insert(it, new_visitor);
[task 2017-04-14T22:43:47.849888Z]                                         ^
[task 2017-04-14T22:43:47.849974Z] /home/worker/workspace/build/src/js/src/jit/arm64/vixl/Decoder-vixl.cpp:133:32: error: ignoring return value of 'bool mozilla::Vector<T, N, AllocPolicy>::append(U&&) [with U = vixl::DecoderVisitor*&; T = vixl::DecoderVisitor*; long unsigned int MinInlineCapacity = 8ul; AllocPolicy = js::SystemAllocPolicy]', declared with attribute warn_unused_result [-Werror=unused-result]
[task 2017-04-14T22:43:47.850004Z]    visitors_.append(new_visitor);
[task 2017-04-14T22:43:47.850022Z]                                 ^
[task 2017-04-14T22:43:47.850061Z] /home/worker/workspace/build/src/js/src/jit/arm64/vixl/Decoder-vixl.cpp: In member function 'void vixl::Decoder::InsertVisitorAfter(vixl::DecoderVisitor*, vixl::DecoderVisitor*)':
[task 2017-04-14T22:43:47.850136Z] /home/worker/workspace/build/src/js/src/jit/arm64/vixl/Decoder-vixl.cpp:142:40: error: ignoring return value of 'T* mozilla::Vector<T, N, AllocPolicy>::insert(T*, U&&) [with U = vixl::DecoderVisitor*&; T = vixl::DecoderVisitor*; long unsigned int MinInlineCapacity = 8ul; AllocPolicy = js::SystemAllocPolicy]', declared with attribute warn_unused_result [-Werror=unused-result]
[task 2017-04-14T22:43:47.850166Z]        visitors_.insert(it, new_visitor);
[task 2017-04-14T22:43:47.850192Z]                                         ^
[task 2017-04-14T22:43:47.850279Z] /home/worker/workspace/build/src/js/src/jit/arm64/vixl/Decoder-vixl.cpp:147:32: error: ignoring return value of 'bool mozilla::Vector<T, N, AllocPolicy>::append(U&&) [with U = vixl::DecoderVisitor*&; T = vixl::DecoderVisitor*; long unsigned int MinInlineCapacity = 8ul; AllocPolicy = js::SystemAllocPolicy]', declared with attribute warn_unused_result [-Werror=unused-result]
[task 2017-04-14T22:43:47.850300Z]    visitors_.append(new_visitor);
[task 2017-04-14T22:43:47.850314Z]                                 ^
[task 2017-04-14T22:43:48.588512Z] /home/worker/workspace/gcc/bin/g++ -std=gnu++11 -o Unified_cpp_js_src26.o -c  -I/home/worker/workspace/build/src/obj-spider/dist/system_wrappers -include /home/worker/workspace/build/src/config/gcc_hidden.h -DDEBUG=1 -DENABLE_BINARYDATA -DENABLE_SIMD -DENABLE_SHARED_ARRAY_BUFFER -DEXPORT_JS_API -DMOZ_HAS_MOZGLUE -I/home/worker/workspace/build/src/js/src -I/home/worker/workspace/build/src/obj-spider/js/src  -I/home/worker/workspace/build/src/obj-spider/dist/include  -I/home/worker/workspace/build/src/obj-spider/dist/include/nspr        -fPIC  -DMOZILLA_CLIENT -include /home/worker/workspace/build/src/obj-spider/js/src/js-confdefs.h -MD -MP -MF .deps/Unified_cpp_js_src26.o.pp  -Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wformat -fno-rtti -fno-exceptions -fno-math-errno -pthread -pipe  -g -freorder-blocks -O3 -fno-omit-frame-pointer  -Werror -Wno-shadow -Werror=format  /home/worker/workspace/build/src/obj-spider/js/src/Unified_cpp_js_src26.cpp
[task 2017-04-14T22:43:49.143378Z] In file included from /home/worker/workspace/build/src/obj-spider/js/src/Unified_cpp_js_src23.cpp:2:0:
[task 2017-04-14T22:43:49.143495Z] /home/worker/workspace/build/src/js/src/jit/arm64/vixl/Instrument-vixl.cpp: In constructor 'vixl::Instrument::Instrument(const char*, uint64_t)':
[task 2017-04-14T22:43:49.143639Z] /home/worker/workspace/build/src/js/src/jit/arm64/vixl/Instrument-vixl.cpp:142:32: error: ignoring return value of 'bool mozilla::Vector<T, N, AllocPolicy>::append(U&&) [with U = vixl::Counter*&; T = vixl::Counter*; long unsigned int MinInlineCapacity = 8ul; AllocPolicy = js::SystemAllocPolicy]', declared with attribute warn_unused_result [-Werror=unused-result]
[task 2017-04-14T22:43:49.143678Z]        counters_.append(counter);
[task 2017-04-14T22:43:49.143710Z]                                 ^
[task 2017-04-14T22:43:49.226021Z] In file included from /home/worker/workspace/build/src/obj-spider/js/src/Unified_cpp_js_src23.cpp:47:0:
[task 2017-04-14T22:43:49.226144Z] /home/worker/workspace/build/src/js/src/jit/arm64/vixl/MozSimulator-vixl.cpp: In member function 'virtual void vixl::Simulator::VisitException(const vixl::Instruction*)':
[task 2017-04-14T22:43:49.226286Z] /home/worker/workspace/build/src/js/src/jit/arm64/vixl/MozSimulator-vixl.cpp:412:57: error: ignoring return value of 'bool mozilla::Vector<T, N, AllocPolicy>::append(U&&) [with U = long int; T = long int; long unsigned int MinInlineCapacity = 0ul; AllocPolicy = js::SystemAllocPolicy]', declared with attribute warn_unused_result [-Werror=unused-result]
[task 2017-04-14T22:43:49.226333Z]            spStack_.append(xreg(31, Reg31IsStackPointer));
[task 2017-04-14T22:43:49.226375Z]                                                          ^
[task 2017-04-14T22:43:50.346434Z] In file included from /home/worker/workspace/build/src/obj-spider/js/src/Unified_cpp_js_src21.cpp:20:0:
[task 2017-04-14T22:43:50.346549Z] /home/worker/workspace/build/src/js/src/jit/arm64/MacroAssembler-arm64.cpp: In member function 'void js::jit::MacroAssembler::callWithABIPre(uint32_t*, bool)':
[task 2017-04-14T22:43:50.346648Z] /home/worker/workspace/build/src/js/src/jit/arm64/MacroAssembler-arm64.cpp:685:32: error: ignoring return value of 'bool js::jit::MoveResolver::resolve()', declared with attribute warn_unused_result [-Werror=unused-result]
[task 2017-04-14T22:43:50.346695Z]          moveResolver_.resolve();
[task 2017-04-14T22:43:50.346727Z]                                 ^
[task 2017-04-14T22:43:54.250775Z] cc1plus: all warnings being treated as errors
[task 2017-04-14T22:43:54.337146Z] /home/worker/workspace/build/src/config/rules.mk:1022: recipe for target 'Unified_cpp_js_src21.o' failed
[task 2017-04-14T22:43:54.337234Z] make[3]: *** [Unified_cpp_js_src21.o] Error 1
[task 2017-04-14T22:43:54.337281Z] make[3]: *** Waiting for unfinished jobs....
[task 2017-04-14T22:43:54.341304Z] make[3]: Entering directory '/home/worker/workspace/build/src/obj-spider/config/external/icu'
[task 2017-04-14T22:43:54.341352Z] libicu.a.desc
[task 2017-04-14T22:43:54.341385Z] rm -f libicu.a
[task 2017-04-14T22:43:54.342127Z] /home/worker/workspace/build/src/obj-spider/_virtualenv/bin/python /home/worker/workspace/build/src/config/expandlibs_gen.py -o libicu.a.desc  ../../../config/external/icu/common/libicuuc.a ../../../config/external/icu/i18n/libicui18n.a ../../../config/external/icu/data/libicudata.a 
[task 2017-04-14T22:43:54.434997Z] make[3]: Leaving directory '/home/worker/workspace/build/src/obj-spider/config/external/icu'
[task 2017-04-14T22:43:55.591125Z] cc1plus: all warnings being treated as errors
[task 2017-04-14T22:43:55.742281Z] /home/worker/workspace/build/src/config/rules.mk:1022: recipe for target 'Unified_cpp_js_src22.o' failed
[task 2017-04-14T22:43:55.742369Z] make[3]: *** [Unified_cpp_js_src22.o] Error 1
[task 2017-04-14T22:43:56.122109Z] cc1plus: all warnings being treated as errors
[task 2017-04-14T22:43:56.225761Z] /home/worker/workspace/build/src/config/rules.mk:1022: recipe for target 'Unified_cpp_js_src23.o' failed
[task 2017-04-14T22:43:56.225844Z] make[3]: *** [Unified_cpp_js_src23.o] Error 1
[task 2017-04-14T22:44:13.504324Z] make[3]: Leaving directory '/home/worker/workspace/build/src/obj-spider/js/src'
[task 2017-04-14T22:44:13.504551Z] /home/worker/workspace/build/src/config/recurse.mk:73: recipe for target 'js/src/target' failed
[task 2017-04-14T22:44:13.504591Z] make[2]: *** [js/src/target] Error 2
[task 2017-04-14T22:44:13.504618Z] make[2]: Leaving directory '/home/worker/workspace/build/src/obj-spider'
[task 2017-04-14T22:44:13.504761Z] /home/worker/workspace/build/src/config/recurse.mk:32: recipe for target 'compile' failed
[task 2017-04-14T22:44:13.504782Z] make[1]: *** [compile] Error 2
[task 2017-04-14T22:44:13.504808Z] make[1]: Leaving directory '/home/worker/workspace/build/src/obj-spider'
[task 2017-04-14T22:44:13.505012Z] /home/worker/workspace/build/src/config/rules.mk:519: recipe for target 'default' failed
[task 2017-04-14T22:44:13.505033Z] make: *** [default] Error 2
Sean - I barely glanced at the code to decide how to handle these.
Attachment #8859008 - Flags: review?(sstangl)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Comment on attachment 8859008 [details] [diff] [review]
Fix uses of uninitialized values in arm64 code

Review of attachment 8859008 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/arm64/vixl/MozSimulator-vixl.cpp
@@ +408,5 @@
>          case kCallRtRedirected:
>            VisitCallRedirection(instr);
>            return;
>          case kMarkStackPointer:
> +          AutoEnterOOMUnsafeRegion oomUnsafe;

/home/worker/workspace/build/src/js/src/jit/arm64/vixl/MozSimulator-vixl.cpp:431:11: error: 'AutoEnterOOMUnsafeRegion' was not declared in this scope

@@ +410,5 @@
>            return;
>          case kMarkStackPointer:
> +          AutoEnterOOMUnsafeRegion oomUnsafe;
> +          if (!spStack_.append(xreg(31, Reg31IsStackPointer)))
> +            oomUnsafe.crash("tracking stack for ARM64 simulator");

/home/worker/workspace/build/src/js/src/jit/arm64/vixl/MozSimulator-vixl.cpp:433:13: error: 'oomUnsafe' was not declared in this scope
Comment on attachment 8859008 [details] [diff] [review]
Fix uses of uninitialized values in arm64 code

Review of attachment 8859008 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/arm64/vixl/Decoder-vixl.cpp
@@ +30,5 @@
>  
>  #include "jit/arm64/vixl/Globals-vixl.h"
>  #include "jit/arm64/vixl/Utils-vixl.h"
>  
> +#include "mozilla/Unused.h"

From check_spidermonkey_style.py:

js/src/jit/arm64/vixl/Decoder-vixl.cpp:32:34: error:
    "jit/arm64/vixl/Utils-vixl.h" should be included after "mozilla/Unused.h"
Yeah, I just slapped that together without testing last night, since it didn't really affect the review. This version compiles and passes check-style.
Attachment #8859300 - Flags: review?(sstangl)
Attachment #8859008 - Attachment is obsolete: true
Attachment #8859008 - Flags: review?(sstangl)
Comment on attachment 8859300 [details] [diff] [review]
Fix uses of uninitialized values in arm64 code

Review of attachment 8859300 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/arm64/vixl/MozSimulator-vixl.cpp
@@ +33,2 @@
>  #include "threading/LockGuard.h"
>  #include "vm/Runtime.h"

FWIW, context changed here in bug 1353763
Comment on attachment 8859300 [details] [diff] [review]
Fix uses of uninitialized values in arm64 code

Review of attachment 8859300 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with minor comment.

::: js/src/jit/arm64/MacroAssembler-arm64.cpp
@@ +683,5 @@
>      reserveStack(*stackAdjust);
>      {
> +        AutoEnterOOMUnsafeRegion oomUnsafe;
> +        if (!moveResolver_.resolve())
> +            oomUnsafe.crash("MacroAssembler::callWithABIPre");

ARM64 has the same enoughMemory_ functionality as x64. Therefore, this can mirror that code as follows:

enoughMemory_ &= moveResolver_.resolve();
if (!enoughMemory_)
    return;

The code paths in the Simulator and the Debugger are OK to crash on OOM since those are just for testing in debug builds, but the MacroAssembler should be more crash-averse.
Attachment #8859300 - Flags: review?(sstangl) → review+
Comment on attachment 8859300 [details] [diff] [review]
Fix uses of uninitialized values in arm64 code

Review of attachment 8859300 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/arm64/vixl/Decoder-vixl.cpp
@@ +113,5 @@
>    }
>  }
>  
>  void Decoder::AppendVisitor(DecoderVisitor* new_visitor) {
> +  mozilla::Unused << visitors_.append(new_visitor);

AFAIK, we use the instruction decoder to emulate JS behaviour in case of asm.js failures.  In such case we are running under the signal handler.

2 questions:
 - Is it safe to allocate?
 - Why don't we check the result of the allocation?
Attachment #8859300 - Attachment is obsolete: true
(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> ::: js/src/jit/arm64/vixl/Decoder-vixl.cpp
> @@ +113,5 @@
> >    }
> >  }
> >  
> >  void Decoder::AppendVisitor(DecoderVisitor* new_visitor) {
> > +  mozilla::Unused << visitors_.append(new_visitor);
> 
> AFAIK, we use the instruction decoder to emulate JS behaviour in case of
> asm.js failures.  In such case we are running under the signal handler.
> 
> 2 questions:
>  - Is it safe to allocate?
>  - Why don't we check the result of the allocation?

That sounds like a question very much outside of my area. If this is running in a signal handler, then I would certainly expect it to not be safe to allocate in the first place.

I don't really know what any of this code does. But from skimming it, my guess is that we only add these visitors during setup type things. Hopefully that doesn't run under a signal handler? But even if it does, there are hardly any calls to these functions:

 - the Debugger adds a single visitor: printer_->AppendVisitor(disasm_)
 - Simulator-vixl.cpp adds two: decoder_->AppendVisitor(instrumentation_) in set_instruction_stats and decoder_->InsertVisitorBefore(print_disasm_, this) in set_trace_parameters
 - MozSimulator-vixl.cpp adds one: decoder_->AppendVisitor(this) in Simulator::init

All these *sound* like singletons, and the visitors_ vector is initialized with inline space for 8 visitors. By that logic, these should all be MOZ_ALWAYS_TRUE. But as I said, I don't really know what's going on here. I'll resubmit for review with that change.
Attachment #8860477 - Flags: review?(sstangl) → review+
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b545af8c568
Fix uses of uninitialized values in arm64 code, r=sstangl
https://hg.mozilla.org/mozilla-central/rev/3b545af8c568
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.