Give PEImage a virtual destructor

Give PEImage a virtual destructor

It has a virtual method and is derived from by PEImageAsData.
Clang would warn about it not having a virtual destructor when
used in a delete expression in
module_integrity_verifier_win_unittest.cc.

Suppress the benign warning about the destructor not being
called during SEH unwinding.

BUG=82385
Committed: https://crrev.com/7d5f646bf9a9c9379cacebf87378e33f29b3ea21
Cr-Commit-Position: refs/heads/master@{#300621}

[hans, 2014-10-21 19:50:53]

hans@chromium.org changed reviewers:
+ robertshield@chromium.org

[hans, 2014-10-21 20:10:26]

Please take a look.

[robertshield, 2014-10-21 20:29:05]

robertshield@chromium.org changed reviewers:
+ cpu@chromium.org

[robertshield, 2014-10-21 20:29:06]

This will fix the warning but I think maybe a broader fix is in order. Would
making operator new/delete private on the PEImage class help prevent the usage
you're changing here?

Also, re. the warning - does the thing the warning technically matter here since
the derived class contains no data members? If not, then perhaps a #pragma to
disable the warning and a big honking comment re. not adding extra members to
derived classes would do as well?

+cpu for opinions

[hans, 2014-10-21 20:37:22]

On 2014/10/21 20:29:06, robertshield wrote:
> This will fix the warning but I think maybe a broader fix is in order. Would
> making operator new/delete private on the PEImage class help prevent the usage
> you're changing here?
> 
> Also, re. the warning - does the thing the warning technically matter here
since
> the derived class contains no data members? If not, then perhaps a #pragma to
> disable the warning and a big honking comment re. not adding extra members to
> derived classes would do as well?

If we're going to suppress warnings, I would rather just give the class a
virtual destructor and suppress the warning about using the object in the
function which uses SEH, since that's sort of the anomaly here. wdyt?

[hans, 2014-10-21 20:50:55]

Uploaded a new version which suppresses the SEH warning instead. I think this is
more sane, since it makes sense for this function to have a virtual dtor.

[robertshield, 2014-10-21 21:10:18]

Cool, this looks much better (and better than what I suggested too :-)). LGTM.

Please check with cpu@ before landing re. disabling the SEH dtor warning. I am
99% sure this is correct, but he can confirm.

[hans, 2014-10-21 21:15:10]

On 2014/10/21 21:10:18, robertshield wrote:
> Cool, this looks much better (and better than what I suggested too :-)). LGTM.
> 
> Please check with cpu@ before landing re. disabling the SEH dtor warning. I am
> 99% sure this is correct, but he can confirm.

Thanks! I'll wait for cpu and have updated the description in the meantime.

[cpu_(ooo_6.6-7.5), 2014-10-22 00:02:26]

lgtm

[hans, 2014-10-22 00:07:09]

The CQ bit was checked by hans@chromium.org

[commit-bot: I haz the power, 2014-10-22 00:08:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/667573009/20001

[commit-bot: I haz the power, 2014-10-22 02:09:16]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-10-22 04:05:29]

Patchset 2 (id:??) landed as
https://crrev.com/7d5f646bf9a9c9379cacebf87378e33f29b3ea21
Cr-Commit-Position: refs/heads/master@{#300621}