search

Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

[TargetLowering] Suspicious code found by PVS Studio #42876

Open Quuxplusone opened 4 years ago

Quuxplusone commented 4 years ago
Bugzilla Link PR43906
Status CONFIRMED
Importance P enhancement
Reported by David Bolvansky (david.bolvansky@gmail.com)
Reported on 2019-11-05 05:04:00 -0800
Last modified on 2021-10-08 19:30:22 -0700
Version trunk
Hardware PC Linux
CC bruno.cardoso@gmail.com, craig.topper@gmail.com, dblaikie@gmail.com, dushistov@gmail.com, francisvm@yahoo.com, llvm-bugs@lists.llvm.org, llvm-dev@redking.me.uk, spatel+llvm@rotateright.com
Fixed by commit(s)
Attachments
Blocks PR30996
Blocked by
See also 
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp   1159    err V763 Parameter
'Offset' is always rewritten in function body before being used.

  // The offset must consider the original displacement from the base symbol
  // since 32-bit targets don't have a GOTPCREL to fold the PC displacement.
  Offset = -MV.getConstant();

Maybe it should be:

  Offset -= MV.getConstant();

?
Quuxplusone commented 4 years ago

Wow - this has been here since 2015!

https://reviews.llvm.org/rL231475

Quuxplusone commented 4 years ago 
Adding this assert:

  assert((Offset == 0 || MV.getConstant() == 0) && "Illegal offset accumulation");
  Offset = -MV.getConstant();

Results in the test failures:

Failing Tests (2):
  LLVM :: MC/MachO/ARM/cstexpr-gotpcrel.ll
  LLVM :: MC/MachO/cstexpr-gotpcrel-32.ll
Quuxplusone commented 4 years ago

Did you try

Offset -= MV.getConstant();

and run tests?

Quuxplusone commented 4 years ago

CC'ing people who have touched those tests.

Quuxplusone commented 4 years ago 
(In reply to David Bolvansky from comment #3)
> Did you try
>
> Offset -= MV.getConstant();
>
> and run tests?

Of course, there are changes but I'm not familiar with the tests and would
prefer someone who is familiar with them to make the changes.
Quuxplusone commented 4 years ago

Hey folks.

Offset -= MV.getConstant(); sounds like different semantics to me. It's been a while since I wrote this so I don't remember the details anymore, but looks like the intention was solely to negate MV.getConstant().

I'm not directly working with this code anymore, but I'm happy to review a change that pass all tests.