630649 - jump bypasses variable initialization

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Fix build with clang.

Clang rejects code like

void* r;
void pushNewFrame(void *);
int match() {
  {
    bool minSatisfied = true;
    pushNewFrame( &&RRETURN_1);
  RRETURN_1: ;
  }
  goto *r;
}

with the error "jump bypasses variable initialization". IMHO the error is reasonable, so the attached patch changes the pcre code. If you think otherwise, let me know why and I will try to change clang instead.

Comment 1

[Boris Zbarsky [:bzbarsky]]

That needs to go upstream too, right?

Comment 2

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Maybe. I noticed that the minSatisfied was recently introduced. I am not sure if it is a mozilla local change or not. If it is not, then yes, this should go upstream too.

Comment 3

[David Mandelin [:dmandelin]]

It is a mozilla-only change. cdleary and I added it to correctly implement ECMA-262 repeated-empty-match semantics.

Comment 4

[Oliver Hunt]

What's the testcase that the change fixed? (it would be useful to have YARR be conformant with the spec upstream -- at least where it's possible)

Comment 5

[David Mandelin [:dmandelin]]

(In reply to comment #4)
> What's the testcase that the change fixed? (it would be useful to have YARR be
> conformant with the spec upstream -- at least where it's possible)

The change was in PCRE--I thought you weren't using it anymore (yarr-interpreter instead). Bug 627609 is the most recent one fixed by these changes: it and its dups have some test cases.

Comment 6

[Oliver Hunt]

Oh, I assumed it was a reference to YARR interpreter (with some significant code style changes :D )

Not sure why i'm  CC'd on this bug :)

(In reply to comment #5)
> (In reply to comment #4)
> > What's the testcase that the change fixed? (it would be useful to have YARR be
> > conformant with the spec upstream -- at least where it's possible)
> 
> The change was in PCRE--I thought you weren't using it anymore
> (yarr-interpreter instead). Bug 627609 is the most recent one fixed by these
> changes: it and its dups have some test cases.

Comment 7

[Nomis101]

This patch doesn't apply anymore, because of the similar changes from Bug 632964.

Comment 8

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

The good news is that that change fixed this bug too :-)

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

so looks like checkin-needed is not needed anymore here, correct if I'm wrong please.