[Lldb-commits] [PATCH] D80955: Fix UB in EmulateInstructionARM64.cpp

2020-06-02 Thread Vedant Kumar via Phabricator via lldb-commits
vsk added inline comments. Comment at: lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp:99 + if (!overflow) +overflow |= !llvm::checkedAdd(*signed_sum, SInt(carry_in)); uint64_t result = unsigned_sum; aprantl wrote: > vsk wrote: > > The d

[Lldb-commits] [PATCH] D80955: Fix UB in EmulateInstructionARM64.cpp

2020-06-01 Thread Adrian Prantl via Phabricator via lldb-commits
This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rGa0b674fd7f06: Fix UB in EmulateInstructionARM64.cpp (authored by aprantl). Herald added a subscriber: mgorny. Herald added

[Lldb-commits] [PATCH] D80955: Fix UB in EmulateInstructionARM64.cpp

2020-06-01 Thread Adrian Prantl via Phabricator via lldb-commits
aprantl added a comment. > This is great Adrian. I feel like there are several other instances of > additions that should use llvm::checkAdd here instead of +. Is this the only > UB triggered in the testsuite? It's the only UB caught by the green dragon / ci.swift.org bots. CHANGES SINCE LAST

[Lldb-commits] [PATCH] D80955: Fix UB in EmulateInstructionARM64.cpp

2020-06-01 Thread Adrian Prantl via Phabricator via lldb-commits
aprantl marked an inline comment as done. aprantl added inline comments. Comment at: lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp:99 + if (!overflow) +overflow |= !llvm::checkedAdd(*signed_sum, SInt(carry_in)); uint64_t result = unsigned_sum;

[Lldb-commits] [PATCH] D80955: Fix UB in EmulateInstructionARM64.cpp

2020-06-01 Thread Davide Italiano via Phabricator via lldb-commits
davide added a comment. This is great Adrian. I feel like there are several other instances of additions that should use `llvm::checkAdd` here instead of `+`. Is this the only UB triggered in the testsuite? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80955/new/ https://reviews.llvm.

[Lldb-commits] [PATCH] D80955: Fix UB in EmulateInstructionARM64.cpp

2020-06-01 Thread Davide Italiano via Phabricator via lldb-commits
davide added inline comments. Comment at: lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp:25 +#include + shafik wrote: > Why? also should be `cstdlib` in C++. It's just something shuffled around, probably a side effect of clang-format. CHAN

[Lldb-commits] [PATCH] D80955: Fix UB in EmulateInstructionARM64.cpp

2020-06-01 Thread Shafik Yaghmour via Phabricator via lldb-commits
shafik added inline comments. Comment at: lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp:25 +#include + Why? also should be `cstdlib` in C++. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80955/new/ https://reviews.llvm.org/D80955

[Lldb-commits] [PATCH] D80955: Fix UB in EmulateInstructionARM64.cpp

2020-06-01 Thread Jason Molenda via Phabricator via lldb-commits
jasonmolenda added a comment. Nice cleanup of a long standing undefined behavior warning. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80955/new/ https://reviews.llvm.org/D80955 ___ lldb-commits mailing list lldb-commits@lists.llvm.org htt

[Lldb-commits] [PATCH] D80955: Fix UB in EmulateInstructionARM64.cpp

2020-06-01 Thread Vedant Kumar via Phabricator via lldb-commits
vsk added a comment. I should add: otherwise, this looks good! Comment at: lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp:99 + if (!overflow) +overflow |= !llvm::checkedAdd(*signed_sum, SInt(carry_in)); uint64_t result = unsigned_sum;

[Lldb-commits] [PATCH] D80955: Fix UB in EmulateInstructionARM64.cpp

2020-06-01 Thread Vedant Kumar via Phabricator via lldb-commits
vsk added a comment. Any chance we can export AddWithCarry via EmulateInstructionARM64.h, then add a unit test in TestArm64InstEmulation.cpp? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80955/new/ https://reviews.llvm.org/D80955 ___ lldb

[Lldb-commits] [PATCH] D80955: Fix UB in EmulateInstructionARM64.cpp

2020-06-01 Thread Adrian Prantl via Phabricator via lldb-commits
aprantl created this revision. aprantl added reviewers: jasonmolenda, vsk. Herald added subscribers: danielkiss, kristof.beyls. This fixes an unhandled signed integer overflow in AddWithCarry() by using the llvm::checkedAdd() function. Thats to @vsk for the suggestion! https://reviews.llvm.org/