The VIXL code in the ARM64 simulator has a number of calls to vanilla operator new, and it also uses std::list and std::vector templates which allocate memory without using js_malloc(). Fixing these issues requires a simple, but fairly sizable, patch which will make it harder to import new VIXL versions from upstream. The test in check_vanilla_allocations.py is failing when building --enable-simulator-arm64 because of these problems. The mechanical changes are: - Replace new char[n] with js_malloc(n) and delete[] p with js_free(p). - Replace new / delete with js_new() / js_delete() templates. - Replace both std::list and std::vector with mozilla::Vector.

Use js_malloc/js_free and js_new/js_delete where appropriate. Replace std::list with mozilla::Vector. The standard list template uses operator new which isn't allowed in SpiderMonkey sources. The Vector is also a more appropriate data structure for these short, infrequently modified lists. Remove the Decoder::visitors() method which was unused and leaking internal representation details anyway.

Use js_malloc/js_free or js_new/js_delete pairs where appropriate. Replace the use of std::vector<Token*> for debugger command arguments with a mozilla::Vector<Token*>. Also use move constructors everywhere for the command arguments instead of passing vector around by value.

Comment on attachment 8663042 [details] [diff] [review] Updated simulator patch: Fix include file order. Review of attachment 8663042 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/arm64/vixl/Decoder-vixl.cpp @@ +148,5 @@ > } > > > void Decoder::RemoveVisitor(DecoderVisitor* visitor) { > + visitors_.erase(std::remove(visitors_.begin(), visitors_.end(), visitor), Why is this necessary, instead of just .erase(visitor)? ::: js/src/jit/arm64/vixl/Instrument-vixl.cpp @@ +148,5 @@ > // Dump any remaining instruction data to the output file. > DumpCounters(); > > // Free all the counter objects. > + for (auto counter : counters_) { I am not sure if we allow this construct in-tree. Sometimes this occurs because we still need to support older compilers that don't support modern C++. Please check (probably with Waldo?) before committing! Although it really should be fine in this particular case, since AArch64 obviously requires a modern compiler in the first place. There are instances where we already say "constexpr" in ARM64-specific code.

Comment on attachment 8663044 [details] [diff] [review] Updated debugger patch: Added #include <jsalloc.h> Review of attachment 8663044 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/arm64/vixl/Debugger-vixl.cpp @@ +1456,5 @@ > } > > > UnknownCommand::~UnknownCommand() { > + for (auto arg : args_) { Same comment as from last patch.

(In reply to Sean Stangl [:sstangl] from comment #5) > Review of attachment 8663042 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jit/arm64/vixl/Decoder-vixl.cpp > @@ +148,5 @@ > > } > > > > > > void Decoder::RemoveVisitor(DecoderVisitor* visitor) { > > + visitors_.erase(std::remove(visitors_.begin(), visitors_.end(), visitor), > > Why is this necessary, instead of just .erase(visitor)? The std::vector template has a remove(x) method which searches for an *element* x and removes all instances. The erase(I) method takes an *iterator* pointing to the instance to be erased. For some reason, mozilla::Vector doesn't have the remove() method, it only has erase(). We need some kind of linear search algorithm to find the visitor to delete. This would also do it: visitors_.erase(std::find(visitors_.begin(), visitors_.end(), visitor)); However, this would only remove the first copy of visitor, and it will be wrong if visitor is not in the array. The erase/remove pattern above is the 'standard' way of removing all instances of an object from an array when the container doesn't provide remove() natively. I agree it's pretty ugly. > > ::: js/src/jit/arm64/vixl/Instrument-vixl.cpp > @@ +148,5 @@ > > // Dump any remaining instruction data to the output file. > > DumpCounters(); > > > > // Free all the counter objects. > > + for (auto counter : counters_) { > > I am not sure if we allow this construct in-tree. Sometimes this occurs > because we still need to support older compilers that don't support modern > C++. Please check (probably with Waldo?) before committing! I was going by the recommendations here: https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code It allows both 'auto' and the range based for loop. > Although it really should be fine in this particular case, since AArch64 > obviously requires a modern compiler in the first place. There are instances > where we already say "constexpr" in ARM64-specific code. There are already examples of auto and range for loops in-tree, both in gc/ and vm/. Thanks!

Hi, could you provide a try run, thanks !

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b721bb31bf83 Note that the SM-tc builds are known to fail for other reasons. See bug 1208118 - SpiderMonkey SM-tc try jobs in TaskCluster failing to find proper compiler Also note that the try server is currently unable to test the ARM64 code. See Bug 1206187 - Create new ARM64 emulator CI job At least we know that this doesn't break the linux64 build. Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f53bcade67e https://hg.mozilla.org/integration/mozilla-inbound/rev/1b689889510b

https://hg.mozilla.org/mozilla-central/rev/8f53bcade67e https://hg.mozilla.org/mozilla-central/rev/1b689889510b