Page MenuHomePhabricator

[Remarks] Add analysis remarks for memset/memcpy/memmove lengths
ClosedPublic

Authored by jroelofs on May 13 2021, 4:48 PM.

Diff Detail

Event Timeline

jroelofs created this revision.May 13 2021, 4:48 PM
jroelofs requested review of this revision.May 13 2021, 4:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2021, 4:48 PM

Maybe additional info about upper bound (if dst or src is local/global buffer; getDerefenceableBytes API) may be interesting too?

Is there anything you would want to share from llvm/lib/Transforms/Utils/AutoInitRemark.cpp ? You can see some examples in llvm/test/Transforms/Util/trivial-auto-var-init-call.ll.

jroelofs updated this revision to Diff 346294.Tue, May 18, 4:10 PM

Unified with the autoinit remarks.

This looks great! Adding Jessica since she reviewed the original auto-init patches too.

xbolva00 added inline comments.Tue, May 18, 4:27 PM
llvm/lib/Transforms/Utils/MemoryOpRemark.cpp
31

Switch over instruction’s opcode?

thegameg accepted this revision.Tue, May 18, 4:28 PM
thegameg added inline comments.
llvm/include/llvm/Transforms/Utils/MemoryOpRemark.h
48

Nit: I am not sure if describe is the best fit here. I used inspect in other places which is probably even worse now that I'm looking back. Maybe something like canHandle?

llvm/lib/Transforms/Utils/MemoryOpRemark.cpp
115

Maybe "Inlined" is better?

This revision is now accepted and ready to land.Tue, May 18, 4:28 PM
jroelofs updated this revision to Diff 346316.Tue, May 18, 5:50 PM
jroelofs marked an inline comment as done.Tue, May 18, 5:53 PM
jroelofs added inline comments.
llvm/include/llvm/Transforms/Utils/MemoryOpRemark.h
48

Other ideas: appliesTo, understands, accepts/accept. I don't have a strong preference.

Also, s/inspect/visit/ seemed appropriate.

llvm/lib/Transforms/Utils/MemoryOpRemark.cpp
31

I'm not sure that would simplify this much, since CallInst/IntrinsicInst aren't leaves.

jroelofs updated this revision to Diff 346493.Wed, May 19, 10:01 AM

Collect object size from the dereferenceable attribute too.

jroelofs updated this revision to Diff 346523.Wed, May 19, 11:47 AM

Collect size information about variables being read from.

jroelofs updated this revision to Diff 346542.Wed, May 19, 1:22 PM

Use TLI instead of string compares on function names.

This revision was landed with ongoing or failed builds.Wed, May 19, 3:19 PM
This revision was automatically updated to reflect the committed changes.
xbolva00 added inline comments.Wed, May 19, 3:51 PM
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
3267

Not related change?

3267

Ok, ignore it. Phab was showing some weird change.

Hi @jroelofs,

we've started getting the warnings for newly created file llvm/lib/Transforms/Utils/MemoryOpRemark.cpp on the cross toolchain builders:

c:\buildbot\as-builder-2\x-aarch64\llvm-project\llvm\lib\transforms\utils\memoryopremark.cpp(123) : warning C4715: 'llvm::MemoryOpRemark::remarkName': not all control paths return a value
c:\buildbot\as-builder-2\x-aarch64\llvm-project\llvm\lib\transforms\utils\memoryopremark.cpp(381) : warning C4715: 'llvm::AutoInitRemark::remarkName': not all control paths return a value

one of the builds as example: https://lab.llvm.org/buildbot/#/builders/119/builds/3889

there is no return for the method's argument unknown value.

Would you take care of it?

Hi @jroelofs,

we've started getting the warnings for newly created file llvm/lib/Transforms/Utils/MemoryOpRemark.cpp on the cross toolchain builders:

c:\buildbot\as-builder-2\x-aarch64\llvm-project\llvm\lib\transforms\utils\memoryopremark.cpp(123) : warning C4715: 'llvm::MemoryOpRemark::remarkName': not all control paths return a value
c:\buildbot\as-builder-2\x-aarch64\llvm-project\llvm\lib\transforms\utils\memoryopremark.cpp(381) : warning C4715: 'llvm::AutoInitRemark::remarkName': not all control paths return a value

one of the builds as example: https://lab.llvm.org/buildbot/#/builders/119/builds/3889

there is no return for the method's argument unknown value.

Would you take care of it?

Sure! This ought to silence it: 3d2ffc88e6afa027369ce100779dacb1c35c6a51