bugzilla.mozilla.org will be intermittently unavailable on Saturday, March 24th, from 16:00 until 20:00 UTC.

"infinite recursion" warning for FloatRegisters::GetName

RESOLVED FIXED in Firefox 50



JavaScript Engine: JIT
2 years ago
2 years ago


(Reporter: froydnj, Assigned: froydnj)



Firefox Tracking Flags

(firefox50 fixed)



(1 attachment)



2 years ago
clang says:

Warning: -Winfinite-recursion in /home/froydnj/src/gecko-dev.git/js/src/jit/arm/Architecture-arm.h: all paths through this function will call itself
/home/froydnj/src/gecko-dev.git/js/src/jit/arm/Architecture-arm.h:277:44: warning: all paths through this function will call itself [-Winfinite-recursion]
    static const char* GetName(uint32_t i) {
The body of the function looks like:

    static const char* GetName(uint32_t i) {
        MOZ_ASSERT(i < Total);
        return GetName(Encoding(i));

and there's no GetName overload.  I guess when we cast |i| to an |Encoding|, it gets implicitly converted back to a |uint32|, and then GetName is called again?

Comment 1

2 years ago
grepping says this function is unused, so we can just delete it!
Assignee: nobody → nfroyd
Oh, we renamed the 2 underlying functions, but not the top-level one, thus the only remaining variant of GetName is the uint32_t variant (which should have been Code instead of uint32_t).

Removing it sounds fine, as it is not needed.

Comment 3

2 years ago
OK, not quite unused.  It is used (without the FloatRegisters prefix), but I have doubts that any of the code that called it actually gets used...

Comment 4

2 years ago
The lone use appears to be here in FloatRegisters::FromName:


which then gets used inside the simulator:


and in VFPRegister::FromName, which I think is unused:


What is the correct fix here?  Should FloatRegisters::FromName actually be checking all the possible results of GetSingleName and GetDoubleName instead?
Flags: needinfo?(nicolas.b.pierron)
I guess this just highlight how unused the arm simulator is for poking at the register content.

 1. This function should take a Code typedef instead of an uint32_t.
 2. We should use the FloatRegister to decode, the kind (isSingle / isDouble) & encoding().
 3. Dispatch to one of the 2 functions with the encoding value.

otherwise MOZ_CRASH is a temporary solution and I can fix it later ;)
Flags: needinfo?(nicolas.b.pierron)

Comment 6

2 years ago
Created attachment 8765547 [details] [diff] [review]
rationalize ARM's FloatRegisters name-accessor code

FloatRegisters::GetName didn't actually do anything, thanks to
type-conversion rules, so modify FloatRegisters::FromName to consider
single and double registers separately.

I'm not entirely sure this is what comment 5 is suggesting; it's not clear to
me what the semantic difference between FloatRegisters::Code and
FloatRegisters::Encoding is, so I made my best stab at it.  If this patch isn't
appropriate, please consider this a request to make FloatRegisters::GetName
call MOZ_CRASH. :)
Attachment #8765547 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8765547 [details] [diff] [review]
rationalize ARM's FloatRegisters name-accessor code

Review of attachment 8765547 [details] [diff] [review]:

Thanks :)

::: js/src/jit/arm/Architecture-arm.cpp
@@ +346,5 @@
>  FloatRegisters::FromName(const char* name)
>  {
> +    for (size_t i = 0; i < TotalSingle; ++i) {
> +        if (strcmp(GetSingleName(Encoding(i)), name) == 0)
> +            return Code(i);

nit: return FloatRegister(i, FloatRegister::Single).code();

@@ +354,1 @@
>              return Code(i);

nit: return FloatRegister(i, FloatRegister::Double).code();
Attachment #8765547 - Flags: review?(nicolas.b.pierron) → review+

Comment 8

2 years ago
Pushed by nfroyd@mozilla.com:
rationalize ARM's FloatRegisters name-accessor code; r=nbp

Comment 9

2 years ago
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.