New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 21 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2015
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug



Sign in to add a comment
link

Issue 167707: Create pe_image 64 bit tests

Reported by cpu@chromium.org, Dec 27 2012 Project Member

Issue description

PEImage is now allowed to compile in 64 bits, but is lacking coverage there.

See codereview.chromium.org/11684007
 

Comment 1 by cpu@chromium.org, Dec 27 2012

Blocking: chromium:8606

Comment 2 by bugdroid1@chromium.org, Jan 4 2013

Project Member
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=175089

------------------------------------------------------------------------
r175089 | jschuh@chromium.org | 2013-01-04T02:40:07.958131Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/win/pe_image.cc?r1=175089&r2=175088&pathrev=175089

Convert 64-bit error for pe_image to #pragma message

I need this to make base compile on Win64. I'll follow-up with fixes in  crbug.com/167707 

Review URL: https://chromiumcodereview.appspot.com/11684007
------------------------------------------------------------------------

Comment 3 by bugdroid1@chromium.org, Feb 13 2013

Project Member
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=182078

------------------------------------------------------------------------
r182078 | jschuh@chromium.org | 2013-02-13T01:15:56.010823Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/installer/util/installer_state_unittest.cc?r1=182078&r2=182077&pathrev=182078
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/win/pe_image_unittest.cc?r1=182078&r2=182077&pathrev=182078

Temporarily disable failing Win64 tests relying on pe_image

64-bit PE files aren't as predictable as 32-bit, so I'll need to modify these tests.
I'm disabling them for now to get the bots on the main waterfall.

R=rvargas@chromium.org,robertshield@chromium.org

BUG= 167707 
Review URL: https://codereview.chromium.org/12207128
------------------------------------------------------------------------

Comment 4 by bugdroid1@chromium.org, Mar 10 2013

Project Member
Labels: -Area-Internals Cr-Internals

Comment 5 by wfh@chromium.org, Apr 30 2013

Labels: Proj-Win64

Comment 6 by jsc...@chromium.org, Apr 14 2014

Owner: wfh@chromium.org
wfh@ - you offered to cover this one.

Comment 7 by jsc...@chromium.org, Apr 14 2014

Blocking: -chromium:8606

Comment 8 by wfh@chromium.org, Jun 9 2014

Labels: Arch-x86_64

Comment 9 by scottmg@chromium.org, Jun 16 2014

Cc: cpu@chromium.org jsc...@chromium.org
This build noise

[16403->235/16671 ~33] CXX obj\base\win\base_static.pe_image.obj
Warning:  This code is not tested on x64. Please make sure all the base unit tests pass before doing any real work. The current unit tests don't test the differences between 32- and 64-bits implementations. Bugs may slip through. You need to improve the coverage before continuing.

is kind of silly. Should this be Release-Block-Stable, or should we delete the #pragma message?

Comment 10 by scottmg@chromium.org, Jun 16 2014

Cc: scottmg@chromium.org

Comment 11 by jsc...@chromium.org, Jun 16 2014

I'm okay with just deleting the pragma because we know our usage is safe. Rick wanted the pragma initially, but that argument is essentially moot now for the same reasons it didn't apply to win32.

That said, lets keep this bug open because Will has a very good plan for improving the test coverage here. He just hasn't had time to implement it.

Comment 12 by wfh@chromium.org, Jun 16 2014

Sure I can push a CL to delete the pragma.  The plan for the tests was to emit some PE and PE64 binaries and verify the headers in the tests.  This is on my (long) todo list.

Comment 13 by rvargas@chromium.org, Jul 8 2014

Cc: rvargas@chromium.org
err... the reason for the warning not applying for x86 was not that our usage was safe (I cannot claim I know how code from base is used), but that I actually followed all the code and made sure it worked as expected, even if it lacked unit tests.

Comment 14 by thakis@chromium.org, Nov 26 2014

Jan 2013: "I need this to make base compile on Win64. I'll follow-up with fixes in  crbug.com/167707 "
Feb 2013: "Temporarily disable failing Win64 tests relying on pe_image" (!)
Aug 2014: chrome 64bit ships

Every 64bit build still warns about this, and apparently we have disabled tests. Something went wrong here.

I'm going to remove the warning as printing it thousands of time each time has no effect (proof: see log above), but it'd be good if someone could look at reenabling the test for this piece of code that we now ship in production.

Comment 15 by bugdroid1@chromium.org, Nov 26 2014

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/092983ab37029417aa4ecc800e27ebcc05c909b5

commit 092983ab37029417aa4ecc800e27ebcc05c909b5
Author: Nico Weber <thakis@chromium.org>
Date: Wed Nov 26 18:15:01 2014

Remove build-time warning from pe_image.cc

This warning has been printed for over 1.5 years on every 64-bit build. Things
like this make people blind to warnings.

The code does look questionable on 64-bit, and the tests for this code are
apparently disabled for 64-bit too. This should be looked at (see bug), but
a warning on every build is not how this will happen.

BUG= 167707 , 82385 
R=scottmg@chromium.org

Review URL: https://codereview.chromium.org/725293009

Cr-Commit-Position: refs/heads/master@{#305834}

[modify] http://crrev.com/092983ab37029417aa4ecc800e27ebcc05c909b5/base/win/pe_image.cc

Comment 16 by thakis@chromium.org, Feb 1 2015

How are tests for this coming along?

Comment 18 by bugdroid1@chromium.org, Feb 25 2015

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b820f17a3767400dac00242c7ee5a948088a0b27

commit b820f17a3767400dac00242c7ee5a948088a0b27
Author: wfh <wfh@chromium.org>
Date: Wed Feb 25 03:02:39 2015

Add base_unittests to win8 trybot. Fix PEImage tests.

Two fixes were required to work on win8:

1. To determine if a directory is a reparse point, use GetFileInformation() since using WIN32_FIND_DATA.dwFileAttributes doesn't work on Windows 8.  See https://msdn.microsoft.com/en-us/library/windows/desktop/aa363940.aspx

2. Update the PE image tests to use its own binary, change the expectation format, add support for Win 8.1 and re-enable on 64-bit.

BUG=373973, 167707 

Review URL: https://codereview.chromium.org/946183002

Cr-Commit-Position: refs/heads/master@{#317960}

[modify] http://crrev.com/b820f17a3767400dac00242c7ee5a948088a0b27/base/BUILD.gn
[modify] http://crrev.com/b820f17a3767400dac00242c7ee5a948088a0b27/base/base.gyp
[modify] http://crrev.com/b820f17a3767400dac00242c7ee5a948088a0b27/base/base_unittests.isolate
[modify] http://crrev.com/b820f17a3767400dac00242c7ee5a948088a0b27/base/files/file_enumerator_win.cc
[add] http://crrev.com/b820f17a3767400dac00242c7ee5a948088a0b27/base/win/pe_image_test.cc
[modify] http://crrev.com/b820f17a3767400dac00242c7ee5a948088a0b27/base/win/pe_image_unittest.cc
[modify] http://crrev.com/b820f17a3767400dac00242c7ee5a948088a0b27/testing/buildbot/chromium_win8_trybot.json

Comment 19 by wfh@chromium.org, Feb 25 2015

Cc: bruening@chromium.org thakis@chromium.org
see also  issue 461816  where building with build_for_tool=drmemory gives different results.

We need to decide how much we care about compilers generating different binaries here.

We have the clear case of clang, which seems like it should be permitted to generate different binaries since it's a different compiler chain completely, but VS should probably remain consistent.

rvargas@ - can you clarify the original purpose of the test - was it to alert us to compiler/linker changes, or was it just to verify that the PEImage code was able to reasonably parse an image with the actual values returned being less important?  If the latter, then making the expectations more flexible to cope with different compiler/linkers seems like a way forward, but if the former, then I can investigate more deeply why build_for_tool=drmemory gives different results, and why clang seems to emit 4 extra relocs.

Comment 20 by wfh@chromium.org, Feb 25 2015

For posterity the output from Clang is:

PEImageTest.EnumeratesPE (run #1):
[ RUN      ] PEImageTest.EnumeratesPE
..\..\base\win\pe_image_unittest.cc(279): error: Value of: count
 Actual: 1590
Expected: expectations.GetExpectation(Expectations::RELOCS)
Which is: 1586
[  FAILED  ] PEImageTest.EnumeratesPE (0 ms)

Comment 21 by rvargas@chromium.org, Feb 25 2015

The tests have nothing to do with what our tools do (they were using OS provided libraries after all). The tests are meant to verify that our PE parsing code works as it should be: that if it says that a file has n froofers, the file really has n froofers.

The wiggle room on the original tests was unfortunate (caused by the fact that the dll changes). If we compile a library so that the values are well known (and stable), maybe we should just check in that dll instead of compiling it with different tools and settings.

Comment 22 by wfh@chromium.org, Feb 25 2015

Cc: brucedaw...@chromium.org

Comment 23 by wfh@chromium.org, Feb 25 2015

Okay I could certainly check in the files (x86 and x64).

Another option would be to run dumpbin /all on the compiled file and then parse out the expected values from there.  This would allow us to determine that the PEImage code is getting the same answers as the toolchain utilities, but it's arguably more complex than it needs to be.

Interestingly, a brief look at how easy it would be to parse dumpbin /all does show some inconsistencies e.g. for a sample binary, dumpbin shows reloc count as:

1A000 [     D18] RVA [size] of Base Relocation Directory

0xD18 = 3352

but PEImage::EnumRelocs tests is showing only counts 1596...?  This probably needs some digging into, or I'm misunderstanding what PEImage is trying to count.

Comment 24 by brucedaw...@chromium.org, Feb 25 2015

The size of 3352 is bytes, so comparing that to a relocation count is not valid. Relocations are, IIRC, a minimum of two bytes, so 1596 relocations is at least 3192 bytes. So, I don't think it is surprising that the two numbers are not directly comparable. I think you'd have to parse dumpbin /relocations in order to compare them.

A ten second look at dumpbin /relocations suggests that the relocations are stored in groups so the extra 160 bytes is presumably the group headers.

Comment 25 by wfh@chromium.org, Feb 25 2015

Thanks Bruce - you're right...

dumpbin /all pe_image_test.dll | grep "ABS\|HIGHLOW" | wc -l

gives 1596.

Comment 26 by wfh@chromium.org, Feb 25 2015

I vote for checking in a binary, and simplifying the code.  Unless I hear dissent, I'll go ahead and implement that.

Comment 27 by brucedaw...@chromium.org, Feb 25 2015

lgtm

Comment 28 by jsc...@chromium.org, Feb 26 2015

sold!

Comment 29 by thakis@chromium.org, Feb 26 2015

On the 64-bit dbg clang bot, it fails slightly differently:

PEImageTest.EnumeratesPE (run #1):
[ RUN      ] PEImageTest.EnumeratesPE
..\..\base\win\pe_image_unittest.cc(271): error: Value of: count
  Actual: 31
Expected: expectations.GetExpectation(Expectations::IMPORTS)
Which is: 69
..\..\base\win\pe_image_unittest.cc(279): error: Value of: count
  Actual: 6
Expected: expectations.GetExpectation(Expectations::RELOCS)
Which is: 632
[  FAILED  ] PEImageTest.EnumeratesPE (0 ms)


Almost all clang testers are red due to this (64 bit release static isn't for some reason). Can you get a fix for this in today, please? Sounds like everyone's on board with prebuilt binaries as test inputs.

Comment 31 by thakis@chromium.org, Feb 27 2015

Status: Started
Thanks, this looks good now.

Is this mostly done? Can the TODO in pe_image.cc be deleted (and maybe the "32 bit specific" comment further down in that file?

Comment 32 by cpu@chromium.org, Jun 6 2015

Cc: -cpu@chromium.org

Comment 33 by rvargas@chromium.org, Sep 24 2015

Cc: -rvargas@chromium.org

Comment 35 by wfh@chromium.org, Dec 24 2015

Status: Fixed

Sign in to add a comment