Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
109483003:
Make sure Chrome_elf.dll imports are correct and that it the first import of chrome.exe (Closed)
|
Created:
7 years ago by Cait (Slow) Modified:
6 years, 11 months ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description1. Make sure chrome_elf.dll imports nothing besides kernel32, advapi32, and some msvc libs (DEBUG builds)
2. Add gyp action and test to ensure chrome_elf.dll is always the first entry in chrome.exe's import table.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242834
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243048
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245197
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245312
Patch Set 1 #
Total comments: 12
Patch Set 2 : Comments #
Total comments: 20
Patch Set 3 : Map file instead of using LoadLibraryEx #
Total comments: 23
Patch Set 4 : Got nits? #
Total comments: 10
Patch Set 5 : rebase on trunk #Patch Set 6 : Roberts comments #Patch Set 7 : Fix one include #Patch Set 8 : #Patch Set 9 : Use swapimports.exe for maintaining import order instead of gyp twiddling #
Total comments: 19
Patch Set 10 : Pythonization and action clean up #Patch Set 11 : More python clean up #
Total comments: 2
Patch Set 12 : A few more nits #
Total comments: 1
Patch Set 13 : parser error nit #Patch Set 14 : rebase on ToT #Patch Set 15 : Disable tests on Winx64 #
Messages
Total messages: 36 (0 generated)
Here are the ChromeELF import tests, so we can at least know if we break things :)
Awesome, thanks for doing this :) Comments and whatnot: https://codereview.chromium.org/109483003/diff/1/chrome_elf/elf_imports_unitt... File chrome_elf/elf_imports_unittest.cc (right): https://codereview.chromium.org/109483003/diff/1/chrome_elf/elf_imports_unitt... chrome_elf/elf_imports_unittest.cc:8: #include <windows.h> nit: C headers before C++ headers https://codereview.chromium.org/109483003/diff/1/chrome_elf/elf_imports_unitt... chrome_elf/elf_imports_unittest.cc:21: "MSVCR100D.dll"}; I think these DLL names change with different run time versions, but I believe that the "MSVC" prefix might be fairly consistent across versions. How about doing a substring match on "msvc*"? I think /base has a wildcard matching utility. https://codereview.chromium.org/109483003/diff/1/chrome_elf/elf_imports_unitt... chrome_elf/elf_imports_unittest.cc:26: for (int i = 0; i<arraysize(kValidELFImports); i++) nit: i < arraysize (spaces) https://codereview.chromium.org/109483003/diff/1/chrome_elf/elf_imports_unitt... chrome_elf/elf_imports_unittest.cc:31: ::LoadLibrary(module_name); LoadLibrary returns the module handle, so you don't need to call GetModuleHandle again. https://codereview.chromium.org/109483003/diff/1/chrome_elf/elf_imports_unitt... chrome_elf/elf_imports_unittest.cc:32: HMODULE module_handle = ::GetModuleHandle(module_name); I think there's also a class in base called base::ScopedNativeLibrary that could be used. https://codereview.chromium.org/109483003/diff/1/chrome_elf/elf_imports_unitt... chrome_elf/elf_imports_unittest.cc:43: (char*) ( (DWORD) module_handle + (DWORD) image_descriptor->Name); no c-style casts: char* name = reinterpret_cast<char*>(module_handle) + reinterpret_cast<char*>(image_descriptor->Name); https://codereview.chromium.org/109483003/diff/1/chrome_elf/elf_imports_unitt... chrome_elf/elf_imports_unittest.cc:44: imports->push_back(std::string(name)); don't think you need to explicitly call this constructor: imports->push_back(name); https://codereview.chromium.org/109483003/diff/1/chrome_elf/elf_imports_unitt... chrome_elf/elf_imports_unittest.cc:45: image_descriptor += sizeof(image_descriptor) / sizeof(uintptr_t); silly question: should this be a uintptr_t or a uint32_t?
Thanks! https://codereview.chromium.org/109483003/diff/1/chrome_elf/elf_imports_unitt... File chrome_elf/elf_imports_unittest.cc (right): https://codereview.chromium.org/109483003/diff/1/chrome_elf/elf_imports_unitt... chrome_elf/elf_imports_unittest.cc:8: #include <windows.h> On 2013/12/09 22:23:13, robertshield wrote: > nit: C headers before C++ headers Done. https://codereview.chromium.org/109483003/diff/1/chrome_elf/elf_imports_unitt... chrome_elf/elf_imports_unittest.cc:21: "MSVCR100D.dll"}; On 2013/12/09 22:23:13, robertshield wrote: > I think these DLL names change with different run time versions, but I believe > that the "MSVC" prefix might be fairly consistent across versions. How about > doing a substring match on "msvc*"? I think /base has a wildcard matching > utility. Done. https://codereview.chromium.org/109483003/diff/1/chrome_elf/elf_imports_unitt... chrome_elf/elf_imports_unittest.cc:26: for (int i = 0; i<arraysize(kValidELFImports); i++) On 2013/12/09 22:23:13, robertshield wrote: > nit: i < arraysize (spaces) Done. https://codereview.chromium.org/109483003/diff/1/chrome_elf/elf_imports_unitt... chrome_elf/elf_imports_unittest.cc:45: image_descriptor += sizeof(image_descriptor) / sizeof(uintptr_t); On 2013/12/09 22:23:13, robertshield wrote: > silly question: should this be a uintptr_t or a uint32_t? I *think* this one should be a uintptr_t (since we want to increment by the size of an IMAGE_IMPORT_DESCRIPTOR ptr each time) -- but I'll run it on an x64 build just to confirm. Update: yes -- uintptr_t is correct here.
On 2013/12/09 23:34:54, Cait Phillips wrote: > Thanks! > > https://codereview.chromium.org/109483003/diff/1/chrome_elf/elf_imports_unitt... > File chrome_elf/elf_imports_unittest.cc (right): > > https://codereview.chromium.org/109483003/diff/1/chrome_elf/elf_imports_unitt... > chrome_elf/elf_imports_unittest.cc:8: #include <windows.h> > On 2013/12/09 22:23:13, robertshield wrote: > > nit: C headers before C++ headers > > Done. > > https://codereview.chromium.org/109483003/diff/1/chrome_elf/elf_imports_unitt... > chrome_elf/elf_imports_unittest.cc:21: "MSVCR100D.dll"}; > On 2013/12/09 22:23:13, robertshield wrote: > > I think these DLL names change with different run time versions, but I believe > > that the "MSVC" prefix might be fairly consistent across versions. How about > > doing a substring match on "msvc*"? I think /base has a wildcard matching > > utility. > > Done. > > https://codereview.chromium.org/109483003/diff/1/chrome_elf/elf_imports_unitt... > chrome_elf/elf_imports_unittest.cc:26: for (int i = 0; > i<arraysize(kValidELFImports); i++) > On 2013/12/09 22:23:13, robertshield wrote: > > nit: i < arraysize (spaces) > > Done. > > https://codereview.chromium.org/109483003/diff/1/chrome_elf/elf_imports_unitt... > chrome_elf/elf_imports_unittest.cc:45: image_descriptor += > sizeof(image_descriptor) / sizeof(uintptr_t); > On 2013/12/09 22:23:13, robertshield wrote: > > silly question: should this be a uintptr_t or a uint32_t? > > I *think* this one should be a uintptr_t (since we want to increment by the size > of an IMAGE_IMPORT_DESCRIPTOR ptr each time) -- but I'll run it on an x64 build > just to confirm. > > Update: yes -- uintptr_t is correct here. ping? :)
Really nice test :) just a couple of nits: https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... File chrome_elf/elf_imports_unittest.cc (right): https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:27: base::ScopedNativeLibrary library(module_handle); This looks odd, but I guess it's done since SNL's interface doesn't actually expose the module handle short of calling Release? bummer. https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:29: if (!module_handle || !imports) checking imports first would allow you to abort early before loading the library https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:56: if (!MatchPattern(*it, kMSVC) && !MatchPattern(*it, kKernel32)) elf is now (or about to) import a few functions from advapi32 as well, could we add these here? also, can the patterns that need to be matched against be expressed as a list to make it easier to add / remove new ones?
nice! https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... File chrome_elf/elf_imports_unittest.cc (right): https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:26: HMODULE module_handle = ::LoadLibrary(module_name); You should be able to do this without executing any code in the image by loading it as a data file, see LoadLibraryEx. https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:32: base::win::PEImage pe_image(module_handle); PeImageAsDatafile, given the above. https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:36: while (image_descriptor->Characteristics) { You can also do this by using one of the PEImage enumeration functions, see PEImage::EnumImportChunks. This has the advantage that it'll do the image parsing for you - it's better to concentrate that sort of stuff in one place IMHO. https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:38: reinterpret_cast<DWORD>(module_handle) + image_descriptor->Name; This is what RVAToAddr does. https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:50: ASSERT_TRUE(elf_imports.size() > 0); ASSERT_LE or GE https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:63: GetImports(L"chrome.exe", &exe_imports); oooh, I don't know what happens when you LoadLibrary an EXE. Better to load it as a data file. https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:66: ASSERT_TRUE(exe_imports.size() > 0); ASSERT_GE or LE?
https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... File chrome_elf/elf_imports_unittest.cc (right): https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:26: HMODULE module_handle = ::LoadLibrary(module_name); On 2013/12/17 14:52:37, Sigurður Ásgeirsson wrote: > You should be able to do this without executing any code in the image by loading > it as a data file, see LoadLibraryEx. Done. https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:27: base::ScopedNativeLibrary library(module_handle); On 2013/12/17 04:02:03, robertshield wrote: > This looks odd, but I guess it's done since SNL's interface doesn't actually > expose the module handle short of calling Release? bummer. Not needed anymore. https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:29: if (!module_handle || !imports) On 2013/12/17 04:02:03, robertshield wrote: > checking imports first would allow you to abort early before loading the library Done. https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:32: base::win::PEImage pe_image(module_handle); On 2013/12/17 14:52:37, Sigurður Ásgeirsson wrote: > PeImageAsDatafile, given the above. Done. https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:36: while (image_descriptor->Characteristics) { On 2013/12/17 14:52:37, Sigurður Ásgeirsson wrote: > You can also do this by using one of the PEImage enumeration functions, see > PEImage::EnumImportChunks. > This has the advantage that it'll do the image parsing for you - it's better to > concentrate that sort of stuff in one place IMHO. Done. https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:38: reinterpret_cast<DWORD>(module_handle) + image_descriptor->Name; On 2013/12/17 14:52:37, Sigurður Ásgeirsson wrote: > This is what RVAToAddr does. Not needed anymore with EnumImportChunks https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:50: ASSERT_TRUE(elf_imports.size() > 0); On 2013/12/17 14:52:37, Sigurður Ásgeirsson wrote: > ASSERT_LE or GE Done. https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:56: if (!MatchPattern(*it, kMSVC) && !MatchPattern(*it, kKernel32)) On 2013/12/17 04:02:03, robertshield wrote: > elf is now (or about to) import a few functions from advapi32 as well, could we > add these here? > > also, can the patterns that need to be matched against be expressed as a list to > make it easier to add / remove new ones? Done. https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:63: GetImports(L"chrome.exe", &exe_imports); On 2013/12/17 14:52:37, Sigurður Ásgeirsson wrote: > oooh, I don't know what happens when you LoadLibrary an EXE. Better to load it > as a data file. Switched to loading the file using CreateFile (as is done here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/t...) since LOAD_LIBRARY_AS_DATAFILE gives a messed up module handle. https://codereview.chromium.org/109483003/diff/20001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:66: ASSERT_TRUE(exe_imports.size() > 0); On 2013/12/17 14:52:37, Sigurður Ásgeirsson wrote: > ASSERT_GE or LE? Done.
nice - I'm glad you found a way not to LoadLibrary the EXE :). Otherwise I have a bunch of nits. https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... File chrome_elf/elf_imports_unittest.cc (right): https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:23: const char* kPatterns[] = { "KERNEL32.dll", "MSVC*", "ADVAPI32.dll" }; nit: since this is used only once, I'd move it to the point of usage, and make it a function static. Also, this is a non-const array of pointers to const char, which is perhaps not exactly what you intended. const char* const kPatterns[] is what you want - this'll make it impossible to (accidentally) stomp on the expectations. Also perhaps e.g. kValidFilePatterns for name. And now I'm all nitted out on this one :). https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:25: bool ImportsCallback(const base::win::PEImage &image, nit: might as well make this a static member of the fixture. For maximal cleanliness, you could make it a privat member, though protected is fine in tests. https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:32: LOG(INFO) << module; did you intend to leave this in? https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:39: void GetImports(const wchar_t* module_name, might as well pass a FilePath in here, it's more explicit. https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:41: if (!imports) ASSERT_TRUE(imports != NULL) or ASSERT_TRUE(imports) is more appropriate in a test IMHO. I'd regard it a programming error to call this function with a NULL imports pointer. https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:44: base::win::ScopedHandle module_file_handle(::CreateFile(module_name, Is base::MemoryMappedFile (base/files/...) not suitable here? I think it may take care of much of the grunge work to close mappings and handles and stuff automatically? https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:54: void* module_file_view = MapViewOfFile( ::MapViewOfFile? https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:57: if (module_file_view != NULL) { Better to ASSERT consistently. This will ensure your test fails on weird conditions, whereas you'd end up succeeding the test on what appears to be an empty imports list if this condition hits. https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:63: module_file_mapping.Close(); This close operation is weirdly scoped, but it's also I think redundant. The ScopedHandles will close when they go out of scope. https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:66: module_file_handle.Close(); also redundant, I think? https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:79: ASSERT_GT(elf_imports.size(), 0u); nit: the ASSERT_XX macros want ASSERT_XX(expected, actual), and they format the output accordingly on failure. Here you want ASSERT_LT, I think? https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:90: if (!match) hint; you can also ASSERT_TRUE(match) << "...."; https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:99: PathService::Get(base::DIR_EXE, &exe); DIR_EXE and DIR_MODULE will produce the same results when you call them from an EXE, as the EXE is the MODULE in this case. I'd go with DIR_EXE in both tests, though I'm not sure what's common practice for tests to locate other test executables. I'm sure CSharp will know, as he's worked on test configuration involving isolates and such...
thanks! https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... File chrome_elf/elf_imports_unittest.cc (right): https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:23: const char* kPatterns[] = { "KERNEL32.dll", "MSVC*", "ADVAPI32.dll" }; On 2013/12/18 21:30:48, Sigurður Ásgeirsson wrote: > nit: since this is used only once, I'd move it to the point of usage, and make > it a function static. > Also, this is a non-const array of pointers to const char, which is perhaps not > exactly what you intended. > > const char* const kPatterns[] > > is what you want - this'll make it impossible to (accidentally) stomp on the > expectations. > > Also perhaps e.g. kValidFilePatterns for name. > > And now I'm all nitted out on this one :). Done. https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:25: bool ImportsCallback(const base::win::PEImage &image, On 2013/12/18 21:30:48, Sigurður Ásgeirsson wrote: > nit: might as well make this a static member of the fixture. For maximal > cleanliness, you could make it a privat member, though protected is fine in > tests. Done. https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:32: LOG(INFO) << module; On 2013/12/18 21:30:48, Sigurður Ásgeirsson wrote: > did you intend to leave this in? oops https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:39: void GetImports(const wchar_t* module_name, On 2013/12/18 21:30:48, Sigurður Ásgeirsson wrote: > might as well pass a FilePath in here, it's more explicit. Done. https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:41: if (!imports) On 2013/12/18 21:30:48, Sigurður Ásgeirsson wrote: > ASSERT_TRUE(imports != NULL) or ASSERT_TRUE(imports) is more appropriate in a > test IMHO. I'd regard it a programming error to call this function with a NULL > imports pointer. Done. https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:44: base::win::ScopedHandle module_file_handle(::CreateFile(module_name, On 2013/12/18 21:30:48, Sigurður Ásgeirsson wrote: > Is base::MemoryMappedFile (base/files/...) not suitable here? > I think it may take care of much of the grunge work to close mappings and > handles and stuff automatically? Yes it is..and it simplifies things a lot -- thanks! https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:54: void* module_file_view = MapViewOfFile( On 2013/12/18 21:30:48, Sigurður Ásgeirsson wrote: > ::MapViewOfFile? Not needed any more https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:79: ASSERT_GT(elf_imports.size(), 0u); On 2013/12/18 21:30:48, Sigurður Ásgeirsson wrote: > nit: the ASSERT_XX macros want ASSERT_XX(expected, actual), and they format the > output accordingly on failure. > > Here you want ASSERT_LT, I think? Done. https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:90: if (!match) On 2013/12/18 21:30:48, Sigurður Ásgeirsson wrote: > hint; you can also > > ASSERT_TRUE(match) << "...."; Done. https://codereview.chromium.org/109483003/diff/40001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:99: PathService::Get(base::DIR_EXE, &exe); On 2013/12/18 21:30:48, Sigurður Ásgeirsson wrote: > DIR_EXE and DIR_MODULE will produce the same results when you call them from an > EXE, as the EXE is the MODULE in this case. I'd go with DIR_EXE in both tests, > though I'm not sure what's common practice for tests to locate other test > executables. I'm sure CSharp will know, as he's worked on test configuration > involving isolates and such... Done.
lgtm - thanks for your patience with my nits, and apologies for my tardiness.
LGTM https://codereview.chromium.org/109483003/diff/60001/chrome_elf/elf_imports_u... File chrome_elf/elf_imports_unittest.cc (right): https://codereview.chromium.org/109483003/diff/60001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:54: PathService::Get(base::DIR_EXE, &dll); ASSERT_TRUE(PathService::Get... https://codereview.chromium.org/109483003/diff/60001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:61: std::vector<std::string>::iterator it = elf_imports.begin(); nit: prefer direct construction vs assignment for non-pod types: std::vector<std::string>::iterator it(elf_imports.begin()); https://codereview.chromium.org/109483003/diff/60001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:68: // Make sure all of ELF's imports are in are valid imports list. are in are -> are in a https://codereview.chromium.org/109483003/diff/60001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:69: for (; it != elf_imports.end(); it++) { it++ -> ++it The latter actually generates more efficient code by avoiding an extra copy especially in debug builds. Though it doesn't make a difference for pod types, it's a good habit to use the prefix operator in general inside for loops to avoid accidentally using a postfix on an iterator. https://codereview.chromium.org/109483003/diff/60001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:83: PathService::Get(base::DIR_EXE, &exe); ASSERT_TRUE(PathService::Get...
thanks for the reviews! https://codereview.chromium.org/109483003/diff/60001/chrome_elf/elf_imports_u... File chrome_elf/elf_imports_unittest.cc (right): https://codereview.chromium.org/109483003/diff/60001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:54: PathService::Get(base::DIR_EXE, &dll); On 2014/01/02 19:36:59, robertshield wrote: > ASSERT_TRUE(PathService::Get... Done. https://codereview.chromium.org/109483003/diff/60001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:61: std::vector<std::string>::iterator it = elf_imports.begin(); On 2014/01/02 19:36:59, robertshield wrote: > nit: prefer direct construction vs assignment for non-pod types: > > std::vector<std::string>::iterator it(elf_imports.begin()); Done. https://codereview.chromium.org/109483003/diff/60001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:68: // Make sure all of ELF's imports are in are valid imports list. On 2014/01/02 19:36:59, robertshield wrote: > are in are -> are in a Done. https://codereview.chromium.org/109483003/diff/60001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:69: for (; it != elf_imports.end(); it++) { On 2014/01/02 19:36:59, robertshield wrote: > it++ -> ++it > > The latter actually generates more efficient code by avoiding an extra copy > especially in debug builds. Though it doesn't make a difference for pod types, > it's a good habit to use the prefix operator in general inside for loops to > avoid accidentally using a postfix on an iterator. Done. https://codereview.chromium.org/109483003/diff/60001/chrome_elf/elf_imports_u... chrome_elf/elf_imports_unittest.cc:83: PathService::Get(base::DIR_EXE, &exe); On 2014/01/02 19:36:59, robertshield wrote: > ASSERT_TRUE(PathService::Get... Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/109483003/140001
Message was sent while issue was closed.
Change committed as 242834
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/109483003/480001
Retried try job too often on win_rel for step(s) nacl_integration http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/109483003/480001
Message was sent while issue was closed.
Change committed as 243048
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/114333008/ by caitkp@chromium.org. The reason for reverting is: Breaking WIN tests on waterfall.
Hi all PTAL (again). No changes to the tests, just a python script to wrap Chris's exe, and a gyp action to call it. Thanks!
+CC csharp for his snake-charming abilities https://codereview.chromium.org/109483003/diff/820001/chrome/chrome_exe.gypi File chrome/chrome_exe.gypi (right): https://codereview.chromium.org/109483003/diff/820001/chrome/chrome_exe.gypi#... chrome/chrome_exe.gypi:8: 'target_name': 'chrome', This should be windows-only right? https://codereview.chromium.org/109483003/diff/820001/chrome/chrome_exe.gypi#... chrome/chrome_exe.gypi:18: 'action_name': 'reodrer_imports', nit: reorder_imports https://codereview.chromium.org/109483003/diff/820001/chrome/chrome_exe.gypi#... chrome/chrome_exe.gypi:22: '<(exe_output_path)', just curious: why does this last one need to be an input? https://codereview.chromium.org/109483003/diff/820001/chrome/chrome_exe.gypi#... chrome/chrome_exe.gypi:40: 'product_name': 'chrome', just curious again: what does 'product_name' do?
https://codereview.chromium.org/109483003/diff/820001/build/win/reorder-impor... File build/win/reorder-imports.py (right): https://codereview.chromium.org/109483003/diff/820001/build/win/reorder-impor... build/win/reorder-imports.py:6: import shutil alphabetize imports https://codereview.chromium.org/109483003/diff/820001/build/win/reorder-impor... build/win/reorder-imports.py:16: (pdbs, manifests etc.).""" Nit: move the ending """ to a newline https://codereview.chromium.org/109483003/diff/820001/build/win/reorder-impor... build/win/reorder-imports.py:26: for fname in glob.glob(os.path.join(input_dir, 'chrome.exe.*')): use glob.iglob so we use an iterator (less memory) https://codereview.chromium.org/109483003/diff/820001/build/win/reorder-impor... build/win/reorder-imports.py:29: Nit: 2 blank lines https://codereview.chromium.org/109483003/diff/820001/build/win/reorder-impor... build/win/reorder-imports.py:34: opts, args = getopt.getopt(argv,"hi:o:",["idir=","odir="]) optparse is easier to use (and more pythonic)
https://codereview.chromium.org/109483003/diff/820001/build/win/reorder-impor... File build/win/reorder-imports.py (right): https://codereview.chromium.org/109483003/diff/820001/build/win/reorder-impor... build/win/reorder-imports.py:6: import shutil On 2014/01/09 21:07:54, csharp wrote: > alphabetize imports Done. https://codereview.chromium.org/109483003/diff/820001/build/win/reorder-impor... build/win/reorder-imports.py:16: (pdbs, manifests etc.).""" On 2014/01/09 21:07:54, csharp wrote: > Nit: move the ending """ to a newline Done. https://codereview.chromium.org/109483003/diff/820001/build/win/reorder-impor... build/win/reorder-imports.py:26: for fname in glob.glob(os.path.join(input_dir, 'chrome.exe.*')): On 2014/01/09 21:07:54, csharp wrote: > use glob.iglob so we use an iterator (less memory) Done. https://codereview.chromium.org/109483003/diff/820001/build/win/reorder-impor... build/win/reorder-imports.py:29: On 2014/01/09 21:07:54, csharp wrote: > Nit: 2 blank lines Done. https://codereview.chromium.org/109483003/diff/820001/build/win/reorder-impor... build/win/reorder-imports.py:34: opts, args = getopt.getopt(argv,"hi:o:",["idir=","odir="]) On 2014/01/09 21:07:54, csharp wrote: > optparse is easier to use (and more pythonic) Done. https://codereview.chromium.org/109483003/diff/820001/chrome/chrome_exe.gypi File chrome/chrome_exe.gypi (right): https://codereview.chromium.org/109483003/diff/820001/chrome/chrome_exe.gypi#... chrome/chrome_exe.gypi:8: 'target_name': 'chrome', On 2014/01/09 20:55:56, robertshield wrote: > This should be windows-only right? Done. https://codereview.chromium.org/109483003/diff/820001/chrome/chrome_exe.gypi#... chrome/chrome_exe.gypi:18: 'action_name': 'reodrer_imports', On 2014/01/09 20:55:56, robertshield wrote: > nit: reorder_imports Done. https://codereview.chromium.org/109483003/diff/820001/chrome/chrome_exe.gypi#... chrome/chrome_exe.gypi:22: '<(exe_output_path)', On 2014/01/09 20:55:56, robertshield wrote: > just curious: why does this last one need to be an input? it doesn't...in fact, it breaks things (passing the exe_input_path as an input breaks things too apparently). I'm still trying to wrap my head around inputs and outputs of gyp actions. As best I can tell, inputs are things that should exist before the action is run, outputs should exist afterwards, and neither is in any way related to the args you actually give to the py script. https://codereview.chromium.org/109483003/diff/820001/chrome/chrome_exe.gypi#... chrome/chrome_exe.gypi:40: 'product_name': 'chrome', On 2014/01/09 20:55:56, robertshield wrote: > just curious again: what does 'product_name' do? product_name makes it chrome.exe rather than chrome_initial.exe
lgtm https://codereview.chromium.org/109483003/diff/820001/chrome/chrome_exe.gypi File chrome/chrome_exe.gypi (right): https://codereview.chromium.org/109483003/diff/820001/chrome/chrome_exe.gypi#... chrome/chrome_exe.gypi:40: 'product_name': 'chrome', On 2014/01/10 22:00:54, Cait Phillips wrote: > On 2014/01/09 20:55:56, robertshield wrote: > > just curious again: what does 'product_name' do? > product_name makes it chrome.exe rather than chrome_initial.exe > Ok, makes sense. Perhaps a comment to this effect? https://codereview.chromium.org/109483003/diff/950001/build/win/reorder-impor... File build/win/reorder-imports.py (right): https://codereview.chromium.org/109483003/diff/950001/build/win/reorder-impor... build/win/reorder-imports.py:22: __file__, '..\\..\\..\\third_party\\syzygy\\binaries\\exe\\swapimport.exe') are in-statement indents 2 or 4 spaces in Chromium's python style? they are 2 here and 4 below in the parser indents, they should be consistent. https://codereview.chromium.org/109483003/diff/950001/chrome_elf/elf_imports_... File chrome_elf/elf_imports_unittest.cc (right): https://codereview.chromium.org/109483003/diff/950001/chrome_elf/elf_imports_... chrome_elf/elf_imports_unittest.cc:38: nit: extra blank line
ping: csharp -- thanks!
LGTM with a small nit https://codereview.chromium.org/109483003/diff/1000001/build/win/reorder-impo... File build/win/reorder-imports.py (right): https://codereview.chromium.org/109483003/diff/1000001/build/win/reorder-impo... build/win/reorder-imports.py:42: print 'Please provide and input and output directory' Nit: You can replace this line and line 43 with: parser.error('Please provide and input and output directory')
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/109483003/1070001
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, base_unittests, base_unittests_swarm, browser_tests, browser_tests_swarm, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, interactive_ui_tests, interactive_ui_tests_swarm, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, net_unittests, net_unittests_swarm, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_unittests, unit_tests, unit_tests_swarm, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/109483003/1070001
Retried try job too often on win_rel for step(s) base_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/109483003/1070001
Message was sent while issue was closed.
Change committed as 245197
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/132613004/ by caitkp@chromium.org. The reason for reverting is: Tests failing on Win x64 bots..
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/109483003/1510001
Message was sent while issue was closed.
Change committed as 245312
Message was sent while issue was closed.
I see [0117/160039:INFO:swapimport_app.cc(201)] Writing output to "..\out\Debug\chrome.exe". [0117/160039:INFO:swapimport_app.cc(216)] Updating output image checksum. Is the pdb for chrome.exe loadable ? in other words, will this confuse crash tools? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
