1017405 - Tag ARM assembler buffer offsets that need translation to a final offset after finishing assembly.

Status: RESOLVED INVALID

(Core :: JavaScript Engine: JIT, defect)

Description

[Douglas Crosher [:dougc]]

For the ARM assembler the buffer locations during assembly may differ after finalizing assembly due to the buffer pools etc. References to buffer locations that are created during assembly, and that need to be referenced after assembly, had been represented by integer offsets that are translated into their final offset by calling actualOffset(). These offsets are not distinguishable and it is very easy for programmers to miss a translation.

One suggestion is to tag these offsets and add some assertion checks to help detect mis-usage.  Offsets into the ARM assembler buffer are all word aligned so the two low bits are available to tag these offsets. In future the offsets might be offset IDs referring to an index into a vector of buffer offset objects, or even a key into a table, so it would be useful to better abstract these offsets now.

Comment 1

[Douglas Crosher [:dougc]]

Attachment: Tag ARM assembler buffer offsets that need translation to a final offset after finishing assembly

Here's an initial try at abstracting the offsets and tagging them. Shall continue testing, and there are a few more code paths I want to scrutinize.

If you have time I could use feedback on the abstraction.

The changes share common ground with those in bug 972710, and if this buffer offset abstraction can be settled it will reduce the patch in bug 972710 which can use the same abstraction.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8430517 [details] [diff] [review]
Tag ARM assembler buffer offsets that need translation to a final offset after finishing assembly

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

Thanks for working on this, would be great to have this fixed.

Passing a (tagged) uint32_t around still seems error-prone though. I'd prefer adding a small class like this:

class Offset // Or OffsetId, BufferOffset etc
{
    uint32_t offset_ : 31;
    bool isActual_ : 1;

  public:
    uint32_t actualOffset() const {
        MOZ_ASSERT(isActual_);
        return offset_;
    }
};

And use it instead of uint32_t everywhere. Basically we want to ensure the only way to get an integer out of this is by calling the right (checked) methods.

Comment 3

[Douglas Crosher [:dougc]]

Could you help me understand this code.  It can use an offset without firstly converting it to an actual offset. This is not causing a known bug, but it frustrates efforts to improve the abstraction. Can the call to resolve() be skipped if a safepoint is not encoded?

CodeGenerator-shared.cpp:

void
CodeGeneratorShared::encodeSafepoints()
{
    for (SafepointIndex *it = safepointIndices_.begin(), *end = safepointIndices_.end();
         it != end;
         ++it)
    {
        LSafepoint *safepoint = it->safepoint();

        if (!safepoint->encoded()) {
            safepoint->fixupOffset(&masm); // << offset is only fixed up if 'encoded()'
            safepoints_.encode(safepoint);
        }

        it->resolve();   // << but the offset seems to be used even if not 'encoded()'?
    }
}

IonFrames-inl.h:
inline void
SafepointIndex::resolve()
{
    JS_ASSERT(!resolved);
    safepointOffset_ = safepoint_->offset();  // << offset is used here, even if not fixed up?
#ifdef DEBUG
    resolved = true;
#endif
}

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Douglas Crosher [:dougc] from comment #3)
>         if (!safepoint->encoded()) {
>             safepoint->fixupOffset(&masm); // << offset is only fixed up if
> 'encoded()'
>             safepoints_.encode(safepoint);
>         }
> 
>         it->resolve();   // << but the offset seems to be used even if not
> 'encoded()'?

As far as I can see, when we call it->resolve(), we should always have called safepoint->fixupOffset(&masm); and encode(..) first, because the "if (!encoded)" is always taken the first time we see a particular safepoint.

fixupOffset does:

    void fixupOffset(MacroAssembler *masm) {
        osiCallPointOffset_ = masm->actualOffset(osiCallPointOffset_);
        safepointOffset_ = masm->actualOffset(safepointOffset_);
    }

So SafepointIndex::resolve() should always see the actual offset. How does it fail, do you get assertion failures? That's unexpected...

Comment 5

[Douglas Crosher [:dougc]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> (In reply to Douglas Crosher [:dougc] from comment #3)
> >         if (!safepoint->encoded()) {
> >             safepoint->fixupOffset(&masm); // << offset is only fixed up if
> > 'encoded()'
> >             safepoints_.encode(safepoint);
> >         }
> > 
> >         it->resolve();   // << but the offset seems to be used even if not
> > 'encoded()'?
> 
> As far as I can see, when we call it->resolve(), we should always have
> called safepoint->fixupOffset(&masm); and encode(..) first, because the "if
> (!encoded)" is always taken the first time we see a particular safepoint.
> 
> fixupOffset does:
> 
>     void fixupOffset(MacroAssembler *masm) {
>         osiCallPointOffset_ = masm->actualOffset(osiCallPointOffset_);
>         safepointOffset_ = masm->actualOffset(safepointOffset_);
>     }
> 
> So SafepointIndex::resolve() should always see the actual offset. How does
> it fail, do you get assertion failures? That's unexpected...

Thank you for the help. The offsets in safepointOffset_ are causing assertion failures when trying to validate them as code offsets. Looking at it again, the safepointOffset_ seems to come from SafepointWriter::startEntry()?  Is it even a code offset, or is it an encoding stream offset? If it's just a stream offset then applying actualOffset() to it would seem inappropriate.

I see some more code to scrutinize around here. Uses of Assembler::patchWrite_NearCallSize(). Shall look into these.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Douglas Crosher [:dougc] from comment #5)
> Looking at
> it again, the safepointOffset_ seems to come from
> SafepointWriter::startEntry()?  Is it even a code offset, or is it an
> encoding stream offset? If it's just a stream offset then applying
> actualOffset() to it would seem inappropriate.

Yes, great catch. That definitely seems wrong.

Comment 7

[Jan de Mooij [:jandem]]

Can you fix that safepointOffset thing in a separate bug, and backport to aurora/beta?

Should be a tiny patch and it could cause weird problems right? (I'm not familiar with the ARM implementation of actualOffset to say for sure.)

Comment 8

[Jan de Mooij [:jandem]]

I think Jakob got rid of the fixup step.