Kannan Vijayan [:djvj] Reporter
	Description • 12 years ago

|icEntryFromPCOffset| is called by the bailout-to-baseline code to figure out the IC for a particular opcode, when it needs to resume into the TypeMonitor chain for some monitored op's IC chain.  This is currently a simple linear search of ICs, finding the first one that matches the pc offset.

However, if the PC being looked up is 0 (which may happen if the first op is a monitored op, is ion compiled, and ion bails on that op due to a typecheck error).. then |icEntryFromPCOffset| will return the UseCount IC, which is wrong.

This method needs to be modified to skip past any incidental start-of-jitcode ICs.  Once bhackett's argument-monitoring patch lands, this will include skipping the initial TypeMonitor ICs that do argument type monitoring.

	Jan de Mooij [:jandem]
	Comment 2 • 12 years ago

Comment on attachment 711149 [details] [diff] [review]
Patch.

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

Good catch.

::: js/src/ion/BaselineIC.h
@@ +204,5 @@
>      // The PC of this IC's bytecode op within the JSScript.
>      uint32_t pcOffset_;
>  
> +    // Whether this IC is for a bytecode op.
> +    uint32_t isForOp_ : 1;

I think this will add a full word to ICEntry. To avoid it, can we change icEntryFromPCOffset to either return the last ICEntry for a pcOffset if there are multiple entries, or test the (fallback) stub kind against a list of stub kinds it should skip?

@@ +214,5 @@
>      ICEntry(uint32_t pcOffset)
> +      : returnOffset_(), pcOffset_(pcOffset), isForOp_(1), firstStub_(NULL)
> +    {}
> +    ICEntry()
> +      : returnOffset_(), pcOffset_(0), isForOp_(0), firstStub_(NULL)

I think we need pcOffset_ != 0 here for the return-address-to-pc mapping to work. Or would we only use this for pcOffset == 0?

	Kannan Vijayan [:djvj] Reporter
	Comment 3 • 12 years ago

> I think this will add a full word to ICEntry. To avoid it, can we change
> icEntryFromPCOffset to either return the last ICEntry for a pcOffset if
> there are multiple entries, or test the (fallback) stub kind against a
> list of stub kinds it should skip?

Sorry, I intended to reduce the bitcount for the pcOffset to 31.  Just in case this is an issue, I changed the baseline compiler to not compile scripts with bytecode length > ~256Million (0xfffffff)

> I think we need pcOffset_ != 0 here for the return-address-to-pc mapping
> to work. Or would we only use this for pcOffset == 0?

No, we have debug trap ICEntries as well.  Technically this is not an issue for bailout since Ion won't be live during debugging, but it feels a bit wrong to rely on that technicality.  I'm fixing this part and just leaving the PC as-is.

	Jan de Mooij [:jandem]
	Comment 4 • 12 years ago

Comment on attachment 711477 [details] [diff] [review]
Try 2.

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

Nice.

	Kannan Vijayan [:djvj] Reporter
	Comment 5 • 12 years ago

https://hg.mozilla.org/projects/ionmonkey/rev/a047df29e0bc