[ARM] GlobalISel: Fix stack-use-after-scope bug.

[mboehme]

Summary

Lifetime extension wasn't triggered on the result of BuildMI because the
reference was non-const. However, instead of adding a const, I've
removed the reference entirely as RVO should kick in anyway.

[bkramer]

lg

[rengolin]

Tests?

[mboehme]

This fixes a bug that was introduced in rL293035. That revision looks as if it provides tests along with the code.

[rengolin]

Though, hum, hard to make tests for that...

OTOH, why push a public review if the review was done internally anyway?

[rengolin]

Also, it would have helped if you had mentioned the commit that introduced the bug, so that it tracks down when people want to revert (the original commit) for some reason.

[bkramer]

In D29124#656224, @rengolin wrote:

Though, hum, hard to make tests for that...

It's covered by existing tests. ASAN catches it with -fsanitize-address-use-after-scope, which is not on by default yet afaik.

OTOH, why push a public review if the review was done internally anyway?

So other people have a chance to chime in? I'd argue that this change is below the triviality threshold for post-commit review though.

[mboehme]

Do you mean why I added rovka@ as a reviewer? My intent was that she would see this, as she's the original author of rL293035. But I take it that's not a usual thing to do? Apologies, still learning how to do things around here.

[mboehme]

In D29124#656227, @rengolin wrote:

Also, it would have helped if you had mentioned the commit that introduced the bug, so that it tracks down when people want to revert (the original commit) for some reason.

Good point -- will do in future!

[rovka]

Thanks, I think this could've gone in without review, since it fixes an obvious bug and is otherwise NFC. I appreciate the intention though ;)

[rengolin]

In D29124#656228, @bkramer wrote:

It's covered by existing tests. ASAN catches it with -fsanitize-address-use-after-scope, which is not on by default yet afaik.

Then say that on the description. What introduced the bug, how did you find out, etc. It helps a lot to know where things came from, not just what they are.

OTOH, why push a public review if the review was done internally anyway?

So other people have a chance to chime in?

Chime in? It was approved seconds after posting and committed one minute after approved. This was clearly an internal process made public for no reason.

I'd argue that this change is below the triviality threshold for post-commit review though.

I'd argue the same thing, but once you posted for review, you *have* to let it linger. This is in the developer policy and it should be followed.

People from the same company, working together, approving on the same minute is really not the way we should be going.

Worst case scenario (you posted by mistake), ping people on email/IRC to expedite the approval. On an obvious patch like this, anyone would have approved instantly.

cheers,
--renato

[mboehme]

In D29124#656235, @rengolin wrote:

Then say that on the description. What introduced the bug, how did you find out, etc. It helps a lot to know where things came from, not just what they are.

Agree that I should have provided this context. Will do so in the future.

I'd argue the same thing, but once you posted for review, you *have* to let it linger. This is in the developer policy and it should be followed.

Apologies -- I was under the impression that post-commit review also involved uploading the patch to Phabricator. I understand now that this is not the case?

My intention here was to obtain a post-commit review from rovka but also to make it clear that bkramer had looked at the change. But I obviously didn't follow the usual process here -- sorry for this, I didn't mean to cause any aggravation or confusion.

[rengolin]

In D29124#656277, @mboehme wrote:

Apologies -- I was under the impression that post-commit review also involved uploading the patch to Phabricator. I understand now that this is not the case?

My intention here was to obtain a post-commit review from rovka but also to make it clear that bkramer had looked at the change. But I obviously didn't follow the usual process here -- sorry for this, I didn't mean to cause any aggravation or confusion.

No worries. No aggravation caused, only confusion. :)

Most of us subscribe to llvm-commits (et al), and filter by the keywords we care ("arm", "aarch64", etc.), so we see all the patches that go though.

If the fix is small and obvious, a direct commit is fine, as long as you explain what you can about what the bug does, how it's being tested, where it came from, etc.

All in all, no hard feelings. :)

cheers,
--reato

[mboehme]

In D29124#656288, @rengolin wrote:

Most of us subscribe to llvm-commits (et al), and filter by the keywords we care ("arm", "aarch64", etc.), so we see all the patches that go though.

If the fix is small and obvious, a direct commit is fine, as long as you explain what you can about what the bug does, how it's being tested, where it came from, etc.

OK, thanks for the explanation!

llvm/trunk/lib/Target/ARM/ARMInstructionSelector.cpp

case 1: {
       if (isSExt) {
         unsigned SExtResult = I.getOperand(0).getReg();
 
         // Use a new virtual register for the result of the AND
         unsigned AndResult = MRI.createVirtualRegister(&ARM::GPRRegClass);
         I.getOperand(0).setReg(AndResult);
 
         auto InsertBefore = std::next(I.getIterator());
-        auto &SubI =
+        auto SubI =
             BuildMI(MBB, InsertBefore, I.getDebugLoc(), TII.get(ARM::RSBri))
                 .addDef(SExtResult)
                 .addUse(AndResult)
                 .addImm(0)
                 .add(predOps(ARMCC::AL))
                 .add(condCodeOp());
         if (!constrainSelectedInstRegOperands(*SubI, TII, TRI, RBI))
           return false;