ensure source position for new instance node is tracked

[adinn]

This is proposed as a simple fix for the problem reported in issue #3620.

[tkrodriguez]

Why not apply a similar fix to other places where VirtualnstanceNode and VirtualArrayNode are allocated? All the ones I can see have the same problem. I don't think changing VirtualizerTool.replaceWithVirtual to set the position is the right fix since that is sometimes used virtual objects that already exist.

[adinn]

@tkrodriguez Thanks for looking at this.

I don't think changing VirtualizerTool.replaceWithVirtual to set the position is the right fix since that is sometimes used virtual objects that already exist.

In that case would we not already have a source node position on the virtual object? i.e. can we not finesse this by setting the source node position only if the current one is either null or a place holder.

I'm assuming that this case only happens when the 'same' virtual object can be reused for multiple virtualizations? If so then leaving the original source node position would be a way to record something useful about the provenance of this 'same' object at all points of use even if it related that provenance to a source location where an original version of that object was created rather than some later source point where an equivalent copy would have been created.

[tkrodriguez]

Yes replaceWithVirtual could only set the value if there wasn't already a value in place. I'd kind of prefer a strategy that forces the creator of VirtualObjectNode to think about it though. We could require a source position argument to the constructor which forces all users to think about it. Or maybe just requiring that argument to VirtualizerTool.createVirtualObject would be less intrusive. Anyway, something more systematic but not too onerous would be better and I'm not overly picky about the particular.

[adinn]

@tkrodriguez Hi Tom.

I just pushed a new fix where I modified createVirtualObject to accept a source position and,if non-null, set it on the VirtualNode (in which case it asserts a position is not already set). I changed all callers to pass what I think is the appropriate source position. There were 2 special cases:

• I had to modify BoxNode to rely on createVirtualObject rather than do the set locally.

• CommitAllocationNode passes null when processing it's virtualObjectList because the position is already set by the constructor for AllocationNode.

Well, there is a third case, actually. I'm not 100% clear I have done the right thing in NewFrameNode. I'm assuming the NewFrameNode has a source position at the point where its virtual references are processed by calling createVirtualObject and propagating the NewFrameNode position to them. I don't really know enough about Truffle to know if this is appropriate or not.

[adinn]

One more tricky case corrected -- ObjectClone should not try to reset the source position when it duplicates an existing virtual node.

[tkrodriguez]

What's here looks good to me. I'm going to be away for a week and a half so I won't be able to give further feedback after today.

[munishchouhan]

@adinn CodeFormatCheck failed in GraalVM Gate / labsjdk-ce-11 style,fullbuild compiler (pull_request) ,

can you please run "mx eclipseformat", check in changes and repush?

gate: 28 Jul 2021 16:10:55(+03:12) ABORT: CodeFormatCheck [0:00:25.080044] [disk (free/total): 36.3GB/83.2GB]
Formatter modified files - run "mx eclipseformat", check in changes and repush

[adinn]

@mcraj017 Hi Munish. I've fixed the style errors and also made the corrections Tom suggested.

[dougxc]

LGTM

[adinn]

@dougxc Thanks for the review!

[adinn]

Hi @dougxc

Is this PR going through the internal gate or has it got stuck for some reason?

[dougxc]

I’m on vacation for 2 weeks but believe @mcraj017 is in the process of merging it.

[adinn]

@dougxc sorry to interrupt your hols. thanks for the update!

[adinn]

@mcraj017 gentle ping: is this still waiting to be merged?

[munishchouhan]

@adinn Yes, still waiting for merge, it will get merged by this week