|
|
DescriptionRefactor DisassemblerWin32X86 and DisassemblerWin32X64 similarities into a base class DisassemblerWin
This is a first attempt to merge similarities between DisassemblerWin32X86 and DisassemblerWin32X64 in a naive way.
This includes creation of the base class and merging of nearly identical function.
BUG=
Committed: https://crrev.com/5ac34c9f0bbd8f8b3c9030a667420fa04d4d8fee
Cr-Commit-Position: refs/heads/master@{#395079}
Patch Set 1 #Patch Set 2 : Add new files to gyp #
Total comments: 20
Patch Set 3 : Merge ParseFileRegion and abolish some wrong-architecture check in unittests #Patch Set 4 : Remove derived ParseFileRegionBody #Patch Set 5 : Renaming disassembler_win for disassembler_win32 to make it work on other platforms #Patch Set 6 : Inject is_PE32_plus_ and offset_of_data_directories_ as constants from derived classes #Patch Set 7 : Change disassemblerWin32 member initialization order to suppress warning #
Total comments: 28
Patch Set 8 : Apply corrections #
Total comments: 16
Patch Set 9 : Add override keyword to destuctor #Patch Set 10 : Some NIT corrections #
Total comments: 16
Patch Set 11 : Running cl format and correcting NITs #Patch Set 12 : Removing redondant stddef.h and replacing bool initialization from 0 to false #Patch Set 13 : Sync #Patch Set 14 : Sync and fix conflicts #
Total comments: 4
Patch Set 15 : Correct formating #
Messages
Total messages: 56 (23 generated)
The CQ bit was checked by etiennep@google.com to run a CQ dry run
etiennep@google.com changed reviewers: + huangs@chromium.org
PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1980703003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1980703003/1
The CQ bit was checked by etiennep@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1980703003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1980703003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
First round. https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... File courgette/disassembler_win.cc (right): https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... courgette/disassembler_win.cc:311: // Skip the relocs that live outside of the image. It might be the case This part of code differs between 32 and 64-bit. In the 64-bit there's a TODO on implementing this. So you're changing functionality. This might be benign, but we should verify that things still work for a reasonably-sized 64-bit EXE (e.g., DLLs from 64-bit Chrome). Since you're testing disassembler only, you can just test with courgette.exe -dis https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... courgette/disassembler_win.cc:324: // Does not occur in Windows x64 executables. Update comment: x86/x64 https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... File courgette/disassembler_win.h (left): https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... courgette/disassembler_win.h:101: bool is_PE32_plus_; // PE32_plus is for 64 bit executables. This should be a "const bool" that's injected by the derived class on construction. Then in ParseHeader() when |magic| is found, emit Error() right away. https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... courgette/disassembler_win.h:106: uint16_t offset_of_data_directories_; offset_of_data_directories_ can also be injected on construction. Alternatively, add virtual uint32_t OffsetOfDataDirectories() = 0 const; and specialize it. https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... File courgette/disassembler_win.h (right): https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... courgette/disassembler_win.h:32: virtual RVA PointerToTargetRVA(const uint8_t* p) const override = 0; Don't need "virtual" here (override suffices). https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... courgette/disassembler_win.h:33: virtual ExecutableType kind() const override = 0; No "virtual". https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... courgette/disassembler_win.h:40: bool is_32bit() const { return !is_PE32_plus_; } Strive to get rid of regular usage of is_32_bit(); 32-bit vs. 64-bit differences should be resolved by inheritance or difference in constant variables. Might keep is_32bit() for test only though -- like mentioned in comment. https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... courgette/disassembler_win.h:41: virtual bool SupportsAddressWidth() = 0; Can probably remove SupportsAddressWidth(). https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... courgette/disassembler_win.h:91: protected: ? https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... File courgette/disassembler_win32_x86.cc (right): https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... courgette/disassembler_win32_x86.cc:111: if (!program->EmitAbs32(program->FindOrMakeAbs32Label(target_rva))) ParseFileRegion() are also common for x86 and x64, except for the 2 lines here: if (!program->EmitAbs32(program->FindOrMakeAbs32Label(target_rva))) ... p += 4; These can use some virtual function / member variable. Here 4 vs. 8 is abs32 target pointer size.
The CQ bit was checked by etiennep@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1980703003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1980703003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by etiennep@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1980703003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1980703003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
The CQ bit was checked by etiennep@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1980703003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1980703003/80001
The CQ bit was checked by etiennep@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1980703003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1980703003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by etiennep@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1980703003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1980703003/120001
PTAnL https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... File courgette/disassembler_win.cc (right): https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... courgette/disassembler_win.cc:311: // Skip the relocs that live outside of the image. It might be the case On 2016/05/13 20:12:20, huangs wrote: > This part of code differs between 32 and 64-bit. In the 64-bit there's a TODO > on implementing this. So you're changing functionality. This might be benign, > but we should verify that things still work for a reasonably-sized 64-bit EXE > (e.g., DLLs from 64-bit Chrome). > > Since you're testing disassembler only, you can just test with > courgette.exe -dis Done. https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... courgette/disassembler_win.cc:324: // Does not occur in Windows x64 executables. On 2016/05/13 20:12:20, huangs wrote: > Update comment: x86/x64 Done. https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... File courgette/disassembler_win.h (left): https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... courgette/disassembler_win.h:101: bool is_PE32_plus_; // PE32_plus is for 64 bit executables. On 2016/05/13 20:12:20, huangs wrote: > This should be a "const bool" that's injected by the derived class on > construction. Then in ParseHeader() when |magic| is found, emit Error() right > away. Actually, it has a different meaning. It is used to tell whether the parsed file is 32bit. We should probably remove it entirely and rely on the type of the disassembler. https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... courgette/disassembler_win.h:106: uint16_t offset_of_data_directories_; On 2016/05/13 20:12:21, huangs wrote: > offset_of_data_directories_ can also be injected on construction. > > Alternatively, add > virtual uint32_t OffsetOfDataDirectories() = 0 const; > and specialize it. Done. https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... File courgette/disassembler_win.h (right): https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... courgette/disassembler_win.h:32: virtual RVA PointerToTargetRVA(const uint8_t* p) const override = 0; On 2016/05/13 20:12:21, huangs wrote: > Don't need "virtual" here (override suffices). Done. https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... courgette/disassembler_win.h:33: virtual ExecutableType kind() const override = 0; On 2016/05/13 20:12:20, huangs wrote: > No "virtual". Done. https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... courgette/disassembler_win.h:40: bool is_32bit() const { return !is_PE32_plus_; } On 2016/05/13 20:12:21, huangs wrote: > Strive to get rid of regular usage of is_32_bit(); 32-bit vs. 64-bit differences > should be resolved by inheritance or difference in constant variables. Might > keep is_32bit() for test only though -- like mentioned in comment. Done. https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... courgette/disassembler_win.h:41: virtual bool SupportsAddressWidth() = 0; On 2016/05/13 20:12:20, huangs wrote: > Can probably remove SupportsAddressWidth(). Done. https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... courgette/disassembler_win.h:91: On 2016/05/13 20:12:21, huangs wrote: > protected: ? We're already in protected section. https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... File courgette/disassembler_win32_x86.cc (right): https://chromiumcodereview.appspot.com/1980703003/diff/20001/courgette/disass... courgette/disassembler_win32_x86.cc:111: if (!program->EmitAbs32(program->FindOrMakeAbs32Label(target_rva))) On 2016/05/13 20:12:21, huangs wrote: > ParseFileRegion() are also common for x86 and x64, except for the 2 lines here: > > if (!program->EmitAbs32(program->FindOrMakeAbs32Label(target_rva))) > ... > p += 4; > > These can use some virtual function / member variable. > Here 4 vs. 8 is abs32 target pointer size. Shouldn't we avoid using a virtual function within the for-loop? EmitAbs32 / EmitAbs64 differs.
More comments. https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... File courgette/disassembler_win32.cc (right): https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32.cc:472: int address_width = AddressWidth(); const int kAddressWidth = ... https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... File courgette/disassembler_win32.h (right): https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32.h:39: bool is_32bit() const { return !is_PE32_plus_; } Can remove is_32bit()? https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32.h:51: RVA Address32ToRVA(uint32_t address) const; Remove. https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32.h:56: explicit DisassemblerWin32(const void* start, Don't need explicit (we do this if and only if there's 1 param). Fix indents (git cl format?) https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32.h:75: // Access address width in byte count. // Returns ... NIT: Maybe rename to AbsVAWidth() or AbsVirtualAddressWidth()? https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32.h:105: std::vector<RVA> abs32_locations_; The stuff here went from private: to protected:, so we should group all member variables together (top), and member functions together. https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32.h:111: const uint16_t offset_of_data_directories_; Make offset_of_data_directories_ returned via virtual accessor instead? https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32.h:112: const bool is_PE32_plus_; // PE32_plus is for 64 bit executables. Remove is_PE32_plus_ ? https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... File courgette/disassembler_win32_x64.h (right): https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32_x64.h:20: explicit DisassemblerWin32X64(const void* start, size_t length); Can remove "explicit". https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32_x64.h:21: ~DisassemblerWin32X64() override = default; https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... File courgette/disassembler_win32_x64_unittest.cc (right): https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32_x64_unittest.cc:65: void DisassemblerWin32X64Test::TestExe32() const { Rename to TextExe32ShouldFail() https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... File courgette/disassembler_win32_x86.h (right): https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32_x86.h:20: explicit DisassemblerWin32X86(const void* start, size_t length); Can remove "explicit". https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32_x86.h:21: ~DisassemblerWin32X86() override = default; https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... File courgette/disassembler_win32_x86_unittest.cc (right): https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32_x86_unittest.cc:65: void DisassemblerWin32X86Test::TestExe64() const { Rename to TestExe64ShouldFail()
The CQ bit was checked by etiennep@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1980703003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1980703003/140001
PTAnL! https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... File courgette/disassembler_win32.cc (right): https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32.cc:472: int address_width = AddressWidth(); On 2016/05/17 17:53:11, huangs wrote: > const int kAddressWidth = ... Done. https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... File courgette/disassembler_win32.h (right): https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32.h:39: bool is_32bit() const { return !is_PE32_plus_; } On 2016/05/17 17:53:11, huangs wrote: > Can remove is_32bit()? Done. https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32.h:51: RVA Address32ToRVA(uint32_t address) const; On 2016/05/17 17:53:11, huangs wrote: > Remove. Done. https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32.h:56: explicit DisassemblerWin32(const void* start, On 2016/05/17 17:53:11, huangs wrote: > Don't need explicit (we do this if and only if there's 1 param). > > Fix indents (git cl format?) Done. https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32.h:75: // Access address width in byte count. On 2016/05/17 17:53:11, huangs wrote: > // Returns ... > NIT: Maybe rename to AbsVAWidth() or AbsVirtualAddressWidth()? Done. https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32.h:105: std::vector<RVA> abs32_locations_; On 2016/05/17 17:53:11, huangs wrote: > The stuff here went from private: to protected:, so we should group all member > variables together (top), and member functions together. Nop https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32.h:111: const uint16_t offset_of_data_directories_; On 2016/05/17 17:53:11, huangs wrote: > Make offset_of_data_directories_ returned via virtual accessor instead? Done. https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32.h:112: const bool is_PE32_plus_; // PE32_plus is for 64 bit executables. On 2016/05/17 17:53:11, huangs wrote: > Remove is_PE32_plus_ ? Ok, I made it into a local variable of Parseheader() https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... File courgette/disassembler_win32_x64.h (right): https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32_x64.h:20: explicit DisassemblerWin32X64(const void* start, size_t length); On 2016/05/17 17:53:11, huangs wrote: > Can remove "explicit". Done. https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32_x64.h:21: On 2016/05/17 17:53:11, huangs wrote: > ~DisassemblerWin32X64() override = default; Done. https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... File courgette/disassembler_win32_x64_unittest.cc (right): https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32_x64_unittest.cc:65: void DisassemblerWin32X64Test::TestExe32() const { On 2016/05/17 17:53:11, huangs wrote: > Rename to TextExe32ShouldFail() Done. https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... File courgette/disassembler_win32_x86.h (right): https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32_x86.h:20: explicit DisassemblerWin32X86(const void* start, size_t length); On 2016/05/17 17:53:11, huangs wrote: > Can remove "explicit". Done. https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32_x86.h:21: On 2016/05/17 17:53:11, huangs wrote: > ~DisassemblerWin32X86() override = default; Done. https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... File courgette/disassembler_win32_x86_unittest.cc (right): https://chromiumcodereview.appspot.com/1980703003/diff/120001/courgette/disas... courgette/disassembler_win32_x86_unittest.cc:65: void DisassemblerWin32X86Test::TestExe64() const { On 2016/05/17 17:53:11, huangs wrote: > Rename to TestExe64ShouldFail() Done.
Only NITS. Please test: - Courgette-gen with production data produces identical result as before. - Courgette-apply works. - Courgette-dis and Courgette-asm works identical as before, for Win32 and Win64 executables. Note that these are not direct inverses; applying Courgette-asm followed by Courgette-dis can discard signature of PE files. https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... File courgette/disassembler_win32.cc (right): https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... courgette/disassembler_win32.cc:167: if (is_PE32_plus) { switch (kind()) { case EXE_WIN_32_X86: ... // Stuff from "else" case EXE_WIN_32_X64: ... // Stuff from "if" default: NOTREACHED(); } ? Can also absorb number_of_data_directories_ = ... below, then get rid of |is_PE32_plus| https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... courgette/disassembler_win32.cc:458: const int address_width = AbsVAWidth(); NIT: kVAWidth (that's the naming convention for consts). https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... File courgette/disassembler_win32.h (right): https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... courgette/disassembler_win32.h:31: FileOffset RVAToFileOffset(RVA rva) const override; NIT: Pass-through declarations: virtual RVA PointerToTargetRVA(const uint8_t* p) const override = 0; virtual ExecutableType kind() const override = 0; https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... courgette/disassembler_win32.h:68: // Emits Abs 32/64 label to the program. Nit: // Emits abs32/abs64 |label| to |program|. https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... courgette/disassembler_win32.h:70: // Returns true is type is recognized. NIT: "Returns true if..." https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... courgette/disassembler_win32.h:115: uint64_t image_base_; // range limited to 32 bits for 32 bit executable Nit: Capitalize "Range". https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... File courgette/disassembler_win32_x64.h (right): https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... courgette/disassembler_win32_x64.h:31: protected: // DisassemblerWin32 interfaces. NIT: Remove new line. https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... File courgette/disassembler_win32_x86.h (right): https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... courgette/disassembler_win32_x86.h:31: protected: // DisassemblerWin32 interfaces. NIT: Remove new line.
I ran gen and apply on for x86 and x64 so far. You can take a look at NIT corrections. https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... File courgette/disassembler_win32.cc (right): https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... courgette/disassembler_win32.cc:167: if (is_PE32_plus) { On 2016/05/17 19:44:33, huangs wrote: > switch (kind()) { > case EXE_WIN_32_X86: > ... // Stuff from "else" > case EXE_WIN_32_X64: > ... // Stuff from "if" > default: > NOTREACHED(); > } > ? > Can also absorb number_of_data_directories_ = ... below, then get rid of > |is_PE32_plus| Done. https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... courgette/disassembler_win32.cc:458: const int address_width = AbsVAWidth(); On 2016/05/17 19:44:33, huangs wrote: > NIT: kVAWidth > (that's the naming convention for consts). Done. https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... File courgette/disassembler_win32.h (right): https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... courgette/disassembler_win32.h:31: FileOffset RVAToFileOffset(RVA rva) const override; On 2016/05/17 19:44:33, huangs wrote: > NIT: Pass-through declarations: > > virtual RVA PointerToTargetRVA(const uint8_t* p) const override = 0; > virtual ExecutableType kind() const override = 0; Done. https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... courgette/disassembler_win32.h:68: // Emits Abs 32/64 label to the program. On 2016/05/17 19:44:33, huangs wrote: > Nit: > // Emits abs32/abs64 |label| to |program|. Done. https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... courgette/disassembler_win32.h:70: // Returns true is type is recognized. On 2016/05/17 19:44:33, huangs wrote: > NIT: "Returns true if..." Done. https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... courgette/disassembler_win32.h:115: uint64_t image_base_; // range limited to 32 bits for 32 bit executable On 2016/05/17 19:44:33, huangs wrote: > Nit: Capitalize "Range". Done. https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... File courgette/disassembler_win32_x64.h (right): https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... courgette/disassembler_win32_x64.h:31: protected: On 2016/05/17 19:44:33, huangs wrote: > // DisassemblerWin32 interfaces. > > NIT: Remove new line. Done. https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... File courgette/disassembler_win32_x86.h (right): https://chromiumcodereview.appspot.com/1980703003/diff/140001/courgette/disas... courgette/disassembler_win32_x86.h:31: protected: On 2016/05/17 19:44:33, huangs wrote: > // DisassemblerWin32 interfaces. > > NIT: Remove new line. Done.
LGTM with NITs. Please run "git cl format" again. Thanks! https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... File courgette/disassembler_win32.cc (right): https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... courgette/disassembler_win32.cc:7: #include <stddef.h> These 2 #includes are redundant. https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... courgette/disassembler_win32.cc:24: incomplete_disassembly_(false), Optional: In C++11 you can directly assign in the .h file. For example, bool incomplete_disassembly_ = false; ... const uint8_t* optional_header_ = nullptr; ... https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... courgette/disassembler_win32.cc:115: return Bad("no PE signature"); NIT: Make capitalization of Bad("...") error messages consistent, e.g., return Bad("No PE signature"); etc. https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... courgette/disassembler_win32.cc:280: return Bad(".relocs outside image"); ... But leave this one alone. https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... File courgette/disassembler_win32_x64.cc (right): https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... courgette/disassembler_win32_x64.cc:7: #include <stddef.h> These 2 #includes are redundant. https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... File courgette/disassembler_win32_x64.h (right): https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... courgette/disassembler_win32_x64.h:17: Should still have class AssemblyProgram; https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... File courgette/disassembler_win32_x86.cc (right): https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... courgette/disassembler_win32_x86.cc:7: #include <stddef.h> These 2 #includes are redundant. https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... File courgette/disassembler_win32_x86.h (right): https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... courgette/disassembler_win32_x86.h:17: Should still have class AssemblyProgram;
The CQ bit was checked by etiennep@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1980703003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1980703003/200001
etiennep@google.com changed reviewers: + wfh@chromium.org
Got LGTM from Samuel, but it would be great to have Will taking a look as well! https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... File courgette/disassembler_win32.cc (right): https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... courgette/disassembler_win32.cc:7: #include <stddef.h> On 2016/05/17 22:12:57, huangs wrote: > These 2 #includes are redundant. Done. https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... courgette/disassembler_win32.cc:24: incomplete_disassembly_(false), On 2016/05/17 22:12:57, huangs wrote: > Optional: In C++11 you can directly assign in the .h file. For example, > > bool incomplete_disassembly_ = false; > ... > const uint8_t* optional_header_ = nullptr; > ... Done. https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... courgette/disassembler_win32.cc:115: return Bad("no PE signature"); On 2016/05/17 22:12:57, huangs wrote: > NIT: Make capitalization of Bad("...") error messages consistent, e.g., > return Bad("No PE signature"); > etc. Done. https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... courgette/disassembler_win32.cc:280: return Bad(".relocs outside image"); On 2016/05/17 22:12:57, huangs wrote: > ... But leave this one alone. Done. https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... File courgette/disassembler_win32_x64.cc (right): https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... courgette/disassembler_win32_x64.cc:7: #include <stddef.h> On 2016/05/17 22:12:57, huangs wrote: > These 2 #includes are redundant. Done. https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... File courgette/disassembler_win32_x64.h (right): https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... courgette/disassembler_win32_x64.h:17: On 2016/05/17 22:12:57, huangs wrote: > Should still have > > class AssemblyProgram; Done. https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... File courgette/disassembler_win32_x86.cc (right): https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... courgette/disassembler_win32_x86.cc:7: #include <stddef.h> On 2016/05/17 22:12:58, huangs wrote: > These 2 #includes are redundant. Done. https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... File courgette/disassembler_win32_x86.h (right): https://chromiumcodereview.appspot.com/1980703003/diff/180001/courgette/disas... courgette/disassembler_win32_x86.h:17: On 2016/05/17 22:12:58, huangs wrote: > Should still have > > class AssemblyProgram; Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by etiennep@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1980703003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1980703003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
Ping wfh@ PTAL.
The CQ bit was checked by etiennep@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1980703003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1980703003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for merging this! Still LGTM, with a few nits. Please run full Courgette-gen and Courgette-apply again to verify.. https://chromiumcodereview.appspot.com/1980703003/diff/260001/courgette/disas... File courgette/disassembler_win32.cc (right): https://chromiumcodereview.appspot.com/1980703003/diff/260001/courgette/disas... courgette/disassembler_win32.cc:350: AssemblyProgram* program) { Weird indent. Run "git cl format" again? https://chromiumcodereview.appspot.com/1980703003/diff/260001/courgette/disas... File courgette/disassembler_win32.h (right): https://chromiumcodereview.appspot.com/1980703003/diff/260001/courgette/disas... courgette/disassembler_win32.h:52: // Disassembler interfaces. NIT: Spacing.
Great, ready to go! https://chromiumcodereview.appspot.com/1980703003/diff/260001/courgette/disas... File courgette/disassembler_win32.cc (right): https://chromiumcodereview.appspot.com/1980703003/diff/260001/courgette/disas... courgette/disassembler_win32.cc:350: AssemblyProgram* program) { On 2016/05/20 05:59:09, huangs wrote: > Weird indent. Run "git cl format" again? I did, Idk how this happened. Anyway, I fixed it! https://chromiumcodereview.appspot.com/1980703003/diff/260001/courgette/disas... File courgette/disassembler_win32.h (right): https://chromiumcodereview.appspot.com/1980703003/diff/260001/courgette/disas... courgette/disassembler_win32.h:52: // Disassembler interfaces. On 2016/05/20 05:59:09, huangs wrote: > NIT: Spacing. Done.
The CQ bit was checked by etiennep@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from huangs@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1980703003/#ps280001 (title: "Correct formating")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1980703003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1980703003/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Refactor DisassemblerWin32X86 and DisassemblerWin32X64 similarities into a base class DisassemblerWin This is a first attempt to merge similarities between DisassemblerWin32X86 and DisassemblerWin32X64 in a naive way. This includes creation of the base class and merging of nearly identical function. BUG= ========== to ========== Refactor DisassemblerWin32X86 and DisassemblerWin32X64 similarities into a base class DisassemblerWin This is a first attempt to merge similarities between DisassemblerWin32X86 and DisassemblerWin32X64 in a naive way. This includes creation of the base class and merging of nearly identical function. BUG= Committed: https://crrev.com/5ac34c9f0bbd8f8b3c9030a667420fa04d4d8fee Cr-Commit-Position: refs/heads/master@{#395079} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/5ac34c9f0bbd8f8b3c9030a667420fa04d4d8fee Cr-Commit-Position: refs/heads/master@{#395079} |