1282096 - "infinite recursion" warning for FloatRegisters::GetName

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nathan Froyd [:froydnj]]

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

[Nathan Froyd [:froydnj]]

grepping says this function is unused, so we can just delete it!

Comment 2

[Nicolas B. Pierron [:nbp]]

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

[Nathan Froyd [:froydnj]]

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

[Nathan Froyd [:froydnj]]

The lone use appears to be here in FloatRegisters::FromName:

http://dxr.mozilla.org/mozilla-central/source/js/src/jit/arm/Architecture-arm.cpp#349

which then gets used inside the simulator:

http://dxr.mozilla.org/mozilla-central/source/js/src/jit/arm/Simulator-arm.cpp#508

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

http://dxr.mozilla.org/mozilla-central/source/js/src/jit/arm/Architecture-arm.h#566

What is the correct fix here?  Should FloatRegisters::FromName actually be checking all the possible results of GetSingleName and GetDoubleName instead?

Comment 5

[Nicolas B. Pierron [:nbp]]

I guess this just highlight how unused the arm simulator is for poking at the register content.

Ideally,
 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 ;)

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: 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. :)

Comment 7

[Nicolas B. Pierron [:nbp]]

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();

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9992f61e7314
rationalize ARM's FloatRegisters name-accessor code; r=nbp

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9992f61e7314