Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12387018:
collect minidump if it chrashes (Closed)
|
Created:
7 years, 9 months ago by edisonn Modified:
7 years, 9 months ago CC:
skia-review_googlegroups.com, bungeman-skia Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionCollect minidump and print callstack if an app chrashes.
Committed: https://code.google.com/p/skia/source/detail?r=8044
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 9
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #Patch Set 14 : #Patch Set 15 : #Patch Set 16 : #Patch Set 17 : #Patch Set 18 : #Patch Set 19 : #Patch Set 20 : #
Total comments: 28
Patch Set 21 : #Patch Set 22 : #Patch Set 23 : #
Total comments: 13
Patch Set 24 : #Patch Set 25 : #
Total comments: 6
Patch Set 26 : #Patch Set 27 : #
Messages
Total messages: 29 (0 generated)
Draft CL. I want to print callstack if a crash is happening instead of just an error number. This will work only in Windows, but it is still better than nothing. Pre requirements: - DbgHelp.lib is used to collect a minidump. Must be on the machine, it is not redistributable so I can't check in this binary on our repository. each buildbot machine must be set up properly - CDB.exe is used to load the minidump and print the callstack. Does not seem to be redistributable either. - we must set this so it is enabled only on buildbot machines (not yet done). Let me know if you have any suggestions.
https://codereview.chromium.org/12387018/diff/11002/tools/win_dbghelp.cpp File tools/win_dbghelp.cpp (right): https://codereview.chromium.org/12387018/diff/11002/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:181: #ifdef _WIN64 is there a better way to get the path to an exe path? I used environment variables, let me know if you think of other solutions in line with current practices.
https://codereview.chromium.org/12387018/diff/11002/gyp/tools.gyp File gyp/tools.gyp (right): https://codereview.chromium.org/12387018/diff/11002/gyp/tools.gyp#newcode248 gyp/tools.gyp:248: 'c:/DbgHelp/DbgHelp.lib', q: can gyp set this path from an environment variable?
On 2013/02/28 16:52:02, edisonn wrote: > Draft CL. > > I want to print callstack if a crash is happening instead of just an error > number. > > This will work only in Windows, but it is still better than nothing. > > Pre requirements: > - DbgHelp.lib is used to collect a minidump. Must be on the machine, it is not > redistributable so I can't check in this binary on our repository. each buildbot > machine must be set up properly > - CDB.exe is used to load the minidump and print the callstack. Does not seem to > be redistributable either. I think these requirements (adding libraries to the Windows system image) will be problematic. Modifying those system images is a pain. What is it about the system image (as opposed to our SVN repository) that makes it ok to distribute via one but not the other? Is it because our SVN repository is world-readable? Maybe we could make a script on the slaves download these libraries automatically, from an internal server? > - we must set this so it is enabled only on buildbot machines (not yet done). What about flipping this logic and enabling it only if the developer sets a special flag? Then the buildbots (and most Windows developers) could keep running just as they are now... and if we see one of these error numbers, we can check out the code at that revision and rebuild it to spit out the callstack.
P.S. Ben Wagner (now in CC) is pretty knowledgable about both Windows and Gyp issues.
I think that this _must_ be opt-in, with a GYP_DEFINE or something. I agree about being careful with adding things to the disk image. Doing so amounts to 6+ hours of work for me, so I like to avoid it whenever possible. However, if this turns out to be very useful, I'm okay with doing it. https://codereview.chromium.org/12387018/diff/11002/gyp/tools.gyp File gyp/tools.gyp (right): https://codereview.chromium.org/12387018/diff/11002/gyp/tools.gyp#newcode179 gyp/tools.gyp:179: 'tools.gyp:win_dbghelp', Please make this opt-in. https://codereview.chromium.org/12387018/diff/11002/gyp/tools.gyp#newcode248 gyp/tools.gyp:248: 'c:/DbgHelp/DbgHelp.lib', On 2013/02/28 17:00:48, edisonn wrote: > q: can gyp set this path from an environment variable? Not directly, that I'm aware of, but it could come from a GYP_DEFINE. https://codereview.chromium.org/12387018/diff/11002/tools/render_pdfs_main.cpp File tools/render_pdfs_main.cpp (right): https://codereview.chromium.org/12387018/diff/11002/tools/render_pdfs_main.cp... tools/render_pdfs_main.cpp:239: int tool_main(int argc, char** argv) { Why not put this in main() rather than adding another layer? https://codereview.chromium.org/12387018/diff/11002/tools/win_dbghelp.cpp File tools/win_dbghelp.cpp (right): https://codereview.chromium.org/12387018/diff/11002/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:181: #ifdef _WIN64 On 2013/02/28 16:56:23, edisonn wrote: > is there a better way to get the path to an exe path? I used environment > variables, let me know if you think of other solutions in line with current > practices. I think an environment variable would be okay. I think the other option might be to add a command-line flag, but that would get cumbersome quickly.
On 2013/02/28 20:28:18, borenet wrote: > I think that this _must_ be opt-in, with a GYP_DEFINE or something. I agree > about being careful with adding things to the disk image. Doing so amounts to > 6+ hours of work for me, so I like to avoid it whenever possible. However, if > this turns out to be very useful, I'm okay with doing it. Maybe we could start out with it being opt-in, and not used by the buildbots at all... and then, as developers get experience using it, we could decide whether to make it happen on the buildbots.
Edi wrote on March 4 (didn't get picked up by the codereview): The only point to print the callstack is so that the buildbot will show that "app failed with callstack" instead of "app failed with error number" and that would show quickly what might be the issue and where to start. There is not point for a developer to use this feature on his machine, just run the app with debugger, and the callstack is there provided quickly. So if you would wish to see the callstack on buildbots instead of an error number, I will go ahead and implement it, but if it does not bring value, I will abandon the CL.
> Edi wrote on March 4 (didn't get picked up by the codereview): > > So if you would wish to see the callstack on buildbots instead of an error > number, I will go ahead and implement it, but if it does not bring value, I will > abandon the CL. I think it would be very useful for the buildbots to show a callstack instead of that useless error number. Please go ahead and commit is as opt-in, so the buildbots don't break without it... then we can try it out on developers' machines to make sure it works as expected, and then decide if/how to get it working on the bots. Sound OK?
On 2013/02/28 19:00:49, epoger wrote: > On 2013/02/28 16:52:02, edisonn wrote: > > Draft CL. > > > > I want to print callstack if a crash is happening instead of just an error > > number. > > > > This will work only in Windows, but it is still better than nothing. > > > > Pre requirements: > > - DbgHelp.lib is used to collect a minidump. Must be on the machine, it is not > > redistributable so I can't check in this binary on our repository. each > buildbot > > machine must be set up properly > > - CDB.exe is used to load the minidump and print the callstack. Does not seem > to > > be redistributable either. > > I think these requirements (adding libraries to the Windows system image) will > be problematic. Modifying those system images is a pain. > > What is it about the system image (as opposed to our SVN repository) that makes > it ok to distribute via one but not the other? Is it because our SVN repository > is world-readable? Maybe we could make a script on the slaves download these > libraries automatically, from an internal server? The "X86/64 Debuggers and Tools_foo.MSI" (installer) is redistributable but the contained cdb.exe and DbgHelp.lib are not. But they are included already in the Widows SDKs which are already installed on the machines. We only need to copy them in a location so we know exactly where they are. Alternatively we might require for them to be in the default path, then the SDK has to be fully installed, but 7.0 and 8.0 paths are different. > > > - we must set this so it is enabled only on buildbot machines (not yet done). > > What about flipping this logic and enabling it only if the developer sets a > special flag? Then the buildbots (and most Windows developers) could keep > running just as they are now... and if we see one of these error numbers, we can > check out the code at that revision and rebuild it to spit out the callstack. the problem is with random failures. Currently render_pdf and bench_pictures fail from time to time with heap corruption. When an error like this occurs, we need the dump and callstack. We don't know when we will hit it again.
FYI: demo on buildbot It shows the callstack of the issue http://70.32.156.53:10115/builders/Skia_Shuttle_Win7_Intel_Float_Release_32_T... On 2013/03/06 16:44:23, edisonn wrote: > On 2013/02/28 19:00:49, epoger wrote: > > On 2013/02/28 16:52:02, edisonn wrote: > > > Draft CL. > > > > > > I want to print callstack if a crash is happening instead of just an error > > > number. > > > > > > This will work only in Windows, but it is still better than nothing. > > > > > > Pre requirements: > > > - DbgHelp.lib is used to collect a minidump. Must be on the machine, it is > not > > > redistributable so I can't check in this binary on our repository. each > > buildbot > > > machine must be set up properly > > > - CDB.exe is used to load the minidump and print the callstack. Does not > seem > > to > > > be redistributable either. > > > > I think these requirements (adding libraries to the Windows system image) will > > be problematic. Modifying those system images is a pain. > > > > What is it about the system image (as opposed to our SVN repository) that > makes > > it ok to distribute via one but not the other? Is it because our SVN > repository > > is world-readable? Maybe we could make a script on the slaves download these > > libraries automatically, from an internal server? > The "X86/64 Debuggers and Tools_foo.MSI" (installer) is redistributable but the > contained cdb.exe and DbgHelp.lib are not. But they are included already in the > Widows SDKs which are already installed on the machines. We only need to copy > them in a location so we know exactly where they are. Alternatively we might > require for them to be in the default path, then the SDK has to be fully > installed, but 7.0 and 8.0 paths are different. > > > > > > - we must set this so it is enabled only on buildbot machines (not yet > done). > > > > What about flipping this logic and enabling it only if the developer sets a > > special flag? Then the buildbots (and most Windows developers) could keep > > running just as they are now... and if we see one of these error numbers, we > can > > check out the code at that revision and rebuild it to spit out the callstack. > the problem is with random failures. Currently render_pdf and bench_pictures > fail from time to time with heap corruption. When an error like this occurs, we > need the dump and callstack. We don't know when we will hit it again.
On 2013/03/06 21:16:18, edisonn wrote: > FYI: demo on buildbot > http://70.32.156.53:10115/builders/Skia_Shuttle_Win7_Intel_Float_Release_32_T... That's very nice! > > The "X86/64 Debuggers and Tools_foo.MSI" (installer) is redistributable but > the > > contained cdb.exe and DbgHelp.lib are not. But they are included already in > the > > Widows SDKs which are already installed on the machines. We only need to copy > > them in a location so we know exactly where they are. Alternatively we might > > require for them to be in the default path, then the SDK has to be fully > > installed, but 7.0 and 8.0 paths are different. So it sounds like there's nothing we need to install, just a path that may need to be set differently on different machines? Sounds like a job for environment variable scripting, which we should be able to do without changing any system images... > > > What about flipping this logic and enabling it only if the developer sets a > > > special flag? Then the buildbots (and most Windows developers) could keep > > > running just as they are now... and if we see one of these error numbers, we > > can > > > check out the code at that revision and rebuild it to spit out the > callstack. > > the problem is with random failures. Currently render_pdf and bench_pictures > > fail from time to time with heap corruption. When an error like this occurs, > we > > need the dump and callstack. We don't know when we will hit it again. I agree that ultimately we want this to be running on the bots. The idea of committing it, but turned off by default, is that it gets us one step in that ultimate direction without breaking anything. Anyway... I see you've been updating this CL... is it ready for re-review? Is there anything in particular to focus on that has changed since patchset 5?
I am still working on the patch, I will send mail when I am ready for review On 2013/03/07 17:05:32, epoger wrote: > On 2013/03/06 21:16:18, edisonn wrote: > > FYI: demo on buildbot > > > http://70.32.156.53:10115/builders/Skia_Shuttle_Win7_Intel_Float_Release_32_T... > > That's very nice! > > > > The "X86/64 Debuggers and Tools_foo.MSI" (installer) is redistributable but > > the > > > contained cdb.exe and DbgHelp.lib are not. But they are included already in > > the > > > Widows SDKs which are already installed on the machines. We only need to > copy > > > them in a location so we know exactly where they are. Alternatively we might > > > require for them to be in the default path, then the SDK has to be fully > > > installed, but 7.0 and 8.0 paths are different. > > So it sounds like there's nothing we need to install, just a path that may need > to be set differently on different machines? Sounds like a job for environment > variable scripting, which we should be able to do without changing any system > images... > > > > > What about flipping this logic and enabling it only if the developer sets > a > > > > special flag? Then the buildbots (and most Windows developers) could keep > > > > running just as they are now... and if we see one of these error numbers, > we > > > can > > > > check out the code at that revision and rebuild it to spit out the > > callstack. > > > the problem is with random failures. Currently render_pdf and bench_pictures > > > fail from time to time with heap corruption. When an error like this occurs, > > we > > > need the dump and callstack. We don't know when we will hit it again. > > I agree that ultimately we want this to be running on the bots. The idea of > committing it, but turned off by default, is that it gets us one step in that > ultimate direction without breaking anything. > > Anyway... I see you've been updating this CL... is it ready for re-review? Is > there anything in particular to focus on that has changed since patchset 5?
Please review https://codereview.chromium.org/12387018/diff/11002/gyp/tools.gyp File gyp/tools.gyp (right): https://codereview.chromium.org/12387018/diff/11002/gyp/tools.gyp#newcode179 gyp/tools.gyp:179: 'tools.gyp:win_dbghelp', On 2013/02/28 20:28:19, borenet wrote: > Please make this opt-in. Done. https://codereview.chromium.org/12387018/diff/11002/gyp/tools.gyp#newcode248 gyp/tools.gyp:248: 'c:/DbgHelp/DbgHelp.lib', On 2013/02/28 20:28:19, borenet wrote: > On 2013/02/28 17:00:48, edisonn wrote: > > q: can gyp set this path from an environment variable? > > Not directly, that I'm aware of, but it could come from a GYP_DEFINE. Done. https://codereview.chromium.org/12387018/diff/11002/tools/render_pdfs_main.cpp File tools/render_pdfs_main.cpp (right): https://codereview.chromium.org/12387018/diff/11002/tools/render_pdfs_main.cp... tools/render_pdfs_main.cpp:239: int tool_main(int argc, char** argv) { On 2013/02/28 20:28:19, borenet wrote: > Why not put this in main() rather than adding another layer? compiler does not like try with all that code d:\edisonn\skia-2\trunk\tools\render_pdfs_main.cpp(238): error C2220: warning treated as error - no 'object' file generated [D:\edisonn\skia-2\trunk\out\gyp\render_pdfs.vcxproj] d:\edisonn\skia-2\trunk\tools\render_pdfs_main.cpp(238): warning C4509: nonstandard extension used: 'tool_main' uses SEH and 'outputDir' has destructor [D:\edisonn\skia-2\trunk\out\gyp\render_pdfs.vc xproj]
https://codereview.chromium.org/12387018/diff/59002/gyp/tools.gyp File gyp/tools.gyp (right): https://codereview.chromium.org/12387018/diff/59002/gyp/tools.gyp#newcode188 gyp/tools.gyp:188: ['skia_win_debuggers_path != ""', This expression can just be: 'skia_win_debuggers_path', which I think is cleaner. https://codereview.chromium.org/12387018/diff/59002/tools/render_pdfs_main.cpp File tools/render_pdfs_main.cpp (right): https://codereview.chromium.org/12387018/diff/59002/tools/render_pdfs_main.cp... tools/render_pdfs_main.cpp:19: #ifdef SK_BUILD_FOR_WIN32 Shouldn't this be #ifdef SK_USE_CDB ? https://codereview.chromium.org/12387018/diff/59002/tools/render_pdfs_main.cp... tools/render_pdfs_main.cpp:252: #endif Can we insert this stuff directly into tool_main without adding another layer? Or maybe add it into main()?
https://codereview.chromium.org/12387018/diff/59002/gyp/tools.gyp File gyp/tools.gyp (right): https://codereview.chromium.org/12387018/diff/59002/gyp/tools.gyp#newcode188 gyp/tools.gyp:188: ['skia_win_debuggers_path != ""', On 2013/03/07 20:22:39, borenet wrote: > This expression can just be: 'skia_win_debuggers_path', which I think is > cleaner. Done. https://codereview.chromium.org/12387018/diff/59002/tools/render_pdfs_main.cpp File tools/render_pdfs_main.cpp (right): https://codereview.chromium.org/12387018/diff/59002/tools/render_pdfs_main.cp... tools/render_pdfs_main.cpp:19: #ifdef SK_BUILD_FOR_WIN32 On 2013/03/07 20:22:39, borenet wrote: > Shouldn't this be #ifdef SK_USE_CDB ? Done. https://codereview.chromium.org/12387018/diff/59002/tools/render_pdfs_main.cp... tools/render_pdfs_main.cpp:252: #endif If I do that, I get compile errors because of the try catch and variable declarations. d:\edisonn\skia-2\trunk\tools\render_pdfs_main.cpp(238): error C2220: warning treated as error - no 'object' file generated [D:\edisonn\skia-2\trunk\out\gyp\render_pdfs.vcxproj] d:\edisonn\skia-2\trunk\tools\render_pdfs_main.cpp(238): warning C4509: nonstandard extension used: 'tool_main' uses SEH and 'outputDir' has destructor [D:\edisonn\skia-2\trunk\out\gyp\render_pdfs.vc xproj] On 2013/03/07 20:22:39, borenet wrote: > Can we insert this stuff directly into tool_main without adding another layer? > Or maybe add it into main()?
https://codereview.chromium.org/12387018/diff/59002/gyp/tools.gyp File gyp/tools.gyp (right): https://codereview.chromium.org/12387018/diff/59002/gyp/tools.gyp#newcode195 gyp/tools.gyp:195: # VS static libraries don't have a linker option. We must set a global project linker option, or add it to each executable. please line-wrap that rascal https://codereview.chromium.org/12387018/diff/59002/tools/win_dbghelp.cpp File tools/win_dbghelp.cpp (right): https://codereview.chromium.org/12387018/diff/59002/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:19: // Using eval feature that evaluates a number in hexa and prints it to stdout hexa -> hex https://codereview.chromium.org/12387018/diff/59002/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:22: #define MARKER_THREAD_CALLSTACK_START_NUMBER "12340000" So I guess you just made up these numbers (12340000, 12340001, etc.), right? And we would search through the output for them in order to see where callstacks start and end? If so, please augment the documentation with something like this... // For example, each thread's callstack will be marked with "12340001" at // the beginning and "12340002" at the end. // We just made up these numbers; they could be anything, as long as they // match up with their decimal equivalents. https://codereview.chromium.org/12387018/diff/59002/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:35: // ? val - evaluarte expression. Used to mark the log file. evaluarte -> evaluate https://codereview.chromium.org/12387018/diff/59002/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:46: strcpy(debug_app_name, app_name); throughout this file: please use stncpy to avoid overflow Seems like you could create a helper function to do it: void strncpyOrSetBlank(char* dest, const char* src) { const char* srcOrEmptyString = (NULL == src) ? "" : src; strncpy(dest, srcOrEmptyString, MAX_PATH); } char debug_app_name[MAX_PATH] = ""; void setAppName(const char* app_name) { strncpyOrSetBlank(debug_app_name, app_name); } https://codereview.chromium.org/12387018/diff/59002/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:90: // the callstack from a log files, only when the application had failed. Please give more information about WHAT this function actually does. What you have noted here is more of an implementation detail, which is good to document but should probably be done INSIDE the function. https://codereview.chromium.org/12387018/diff/59002/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:97: while (fgets(line, sizeof(line), file)) { Do we need to worry about lines longer than 1000 characters? What happens if this code encounters one? https://codereview.chromium.org/12387018/diff/59002/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:103: // Filter messages. Calstack lines contain "exe/dll!function" Calstack -> Callstack More importantly, I'm confused about what form these lines take. Do they show "exe/dll!function" or "ffffeeee ffffdddd!function" ? Please replace magic number 18 with some sample string(s) constant and strlen math. https://codereview.chromium.org/12387018/diff/59002/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:113: int GenerateDumpAndPrintCallstack(EXCEPTION_POINTERS* pExceptionPointers) { please document meaning of return value https://codereview.chromium.org/12387018/diff/59002/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:137: sprintf( szFileNameOutput, "%s%s\\%s-%04d%02d%02d-%02d%02d%02d-%ld-%ld.out", It looks like this formatting string has a lot in common with the one above. Maybe build them as preprocessor macros? https://codereview.chromium.org/12387018/diff/59002/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:204: (vargs0[i - 1] == 'E' || vargs0[i - 1] == 'e') && on windows, I think you can use stricmp() http://msdn.microsoft.com/en-us/library/k59z8dwe(v=vs.80).aspx
https://codereview.chromium.org/12387018/diff/59002/gyp/tools.gyp File gyp/tools.gyp (right): https://codereview.chromium.org/12387018/diff/59002/gyp/tools.gyp#newcode195 gyp/tools.gyp:195: # VS static libraries don't have a linker option. We must set a global project linker option, or add it to each executable. On 2013/03/07 21:09:09, epoger wrote: > please line-wrap that rascal Done. https://codereview.chromium.org/12387018/diff/59002/tools/win_dbghelp.cpp File tools/win_dbghelp.cpp (right): https://codereview.chromium.org/12387018/diff/59002/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:19: // Using eval feature that evaluates a number in hexa and prints it to stdout On 2013/03/07 21:09:09, epoger wrote: > hexa -> hex Done. https://codereview.chromium.org/12387018/diff/59002/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:22: #define MARKER_THREAD_CALLSTACK_START_NUMBER "12340000" On 2013/03/07 21:09:09, epoger wrote: > So I guess you just made up these numbers (12340000, 12340001, etc.), right? > And we would search through the output for them in order to see where callstacks > start and end? > > If so, please augment the documentation with something like this... > > // For example, each thread's callstack will be marked with "12340001" at > // the beginning and "12340002" at the end. > // We just made up these numbers; they could be anything, as long as they > // match up with their decimal equivalents. Done. https://codereview.chromium.org/12387018/diff/59002/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:35: // ? val - evaluarte expression. Used to mark the log file. On 2013/03/07 21:09:09, epoger wrote: > evaluarte -> evaluate Done. https://codereview.chromium.org/12387018/diff/59002/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:46: strcpy(debug_app_name, app_name); On 2013/03/07 21:09:09, epoger wrote: > throughout this file: please use stncpy to avoid overflow > > Seems like you could create a helper function to do it: > > void strncpyOrSetBlank(char* dest, const char* src) { > const char* srcOrEmptyString = (NULL == src) ? "" : src; > strncpy(dest, srcOrEmptyString, MAX_PATH); > } > > char debug_app_name[MAX_PATH] = ""; > void setAppName(const char* app_name) { > strncpyOrSetBlank(debug_app_name, app_name); > } Done. https://codereview.chromium.org/12387018/diff/59002/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:90: // the callstack from a log files, only when the application had failed. On 2013/03/07 21:09:09, epoger wrote: > Please give more information about WHAT this function actually does. What you > have noted here is more of an implementation detail, which is good to document > but should probably be done INSIDE the function. Done. https://codereview.chromium.org/12387018/diff/59002/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:97: while (fgets(line, sizeof(line), file)) { We will print the frame truncated, or not print it at all. I am not worried bu 1000 character function/dll/exe names. On 2013/03/07 21:09:09, epoger wrote: > Do we need to worry about lines longer than 1000 characters? What happens if > this code encounters one? https://codereview.chromium.org/12387018/diff/59002/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:103: // Filter messages. Calstack lines contain "exe/dll!function" lines are like "ffffeeee ffffdddd render_pdf!processInput" +done On 2013/03/07 21:09:09, epoger wrote: > Calstack -> Callstack > > More importantly, I'm confused about what form these lines take. Do they show > "exe/dll!function" or "ffffeeee ffffdddd!function" ? > > Please replace magic number 18 with some sample string(s) constant and strlen > math. https://codereview.chromium.org/12387018/diff/59002/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:113: int GenerateDumpAndPrintCallstack(EXCEPTION_POINTERS* pExceptionPointers) { On 2013/03/07 21:09:09, epoger wrote: > please document meaning of return value Done. https://codereview.chromium.org/12387018/diff/59002/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:137: sprintf( szFileNameOutput, "%s%s\\%s-%04d%02d%02d-%02d%02d%02d-%ld-%ld.out", On 2013/03/07 21:09:09, epoger wrote: > It looks like this formatting string has a lot in common with the one above. > Maybe build them as preprocessor macros? Done. https://codereview.chromium.org/12387018/diff/59002/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:204: (vargs0[i - 1] == 'E' || vargs0[i - 1] == 'e') && On 2013/03/07 21:09:09, epoger wrote: > on windows, I think you can use stricmp() > > http://msdn.microsoft.com/en-us/library/k59z8dwe%28v=vs.80%29.aspx Done.
LGTM (dunno if Eric still has issues, though...) Thanks for the cleanup! https://codereview.chromium.org/12387018/diff/63003/tools/win_dbghelp.cpp File tools/win_dbghelp.cpp (right): https://codereview.chromium.org/12387018/diff/63003/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:18: // e.g. "01234567 01234567 render_pdf!processInput General suggestion, nothing necessarily needed here: IMHO, when we are making up hexadecimal values, I would recommend including at least some a-f digits to make it clearer that it's hexadecimal! https://codereview.chromium.org/12387018/diff/63003/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:49: static void strncpyOrSetBlank(char* dest, const char* src, size_t len) { I had hoped that, if you passed in dest as char[] instead of char *, you could move the sizeof(dest) into this function. But you can't. :-( Details at http://stackoverflow.com/questions/37538/how-do-i-determine-the-size-of-my-ar... If you want to eliminate the hazard of passing the wrong len value, I guess you could make a macro like this: #define STRNCPY_OR_SET_BLANK(dest, src) strnpyOrSetBlank((dest), (src), sizeof(dest))
LGTM with nits. https://codereview.chromium.org/12387018/diff/63003/gyp/tools.gyp File gyp/tools.gyp (right): https://codereview.chromium.org/12387018/diff/63003/gyp/tools.gyp#newcode195 gyp/tools.gyp:195: # VS static libraries don't have a linker option. We must set a global project Nit: I'd still prefer if we stayed under the column limit here. https://codereview.chromium.org/12387018/diff/63003/gyp/tools.gyp#newcode197 gyp/tools.gyp:197: ['skia_win_debuggers_path and skia_os == "win" and skia_arch_width == 64', Nit: and here https://codereview.chromium.org/12387018/diff/63003/gyp/tools.gyp#newcode208 gyp/tools.gyp:208: ['skia_win_debuggers_path and skia_os == "win" and skia_arch_width == 32', Nit: and here https://codereview.chromium.org/12387018/diff/63003/tools/render_pdfs_main.cpp File tools/render_pdfs_main.cpp (right): https://codereview.chromium.org/12387018/diff/63003/tools/render_pdfs_main.cp... tools/render_pdfs_main.cpp:211: } Nit: lines ending in spaces
https://codereview.chromium.org/12387018/diff/63003/tools/win_dbghelp.cpp File tools/win_dbghelp.cpp (right): https://codereview.chromium.org/12387018/diff/63003/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:18: // e.g. "01234567 01234567 render_pdf!processInput On 2013/03/08 16:12:42, epoger wrote: > General suggestion, nothing necessarily needed here: IMHO, when we are making up > hexadecimal values, I would recommend including at least some a-f digits to make > it clearer that it's hexadecimal! Done. https://codereview.chromium.org/12387018/diff/63003/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:49: static void strncpyOrSetBlank(char* dest, const char* src, size_t len) { the problem is that someone might allocate dynamically the string in a new version, and then mess up the strcpy. I prefer to keep it explicitly. But we could switch to using language D which has goodies like that AFAIK On 2013/03/08 16:12:42, epoger wrote: > I had hoped that, if you passed in dest as char[] instead of char *, you could > move the sizeof(dest) into this function. But you can't. :-( Details at > http://stackoverflow.com/questions/37538/how-do-i-determine-the-size-of-my-ar... > > If you want to eliminate the hazard of passing the wrong len value, I guess you > could make a macro like this: > > #define STRNCPY_OR_SET_BLANK(dest, src) strnpyOrSetBlank((dest), (src), > sizeof(dest))
https://codereview.chromium.org/12387018/diff/63003/gyp/tools.gyp File gyp/tools.gyp (right): https://codereview.chromium.org/12387018/diff/63003/gyp/tools.gyp#newcode195 gyp/tools.gyp:195: # VS static libraries don't have a linker option. We must set a global project On 2013/03/08 16:30:56, borenet wrote: > Nit: I'd still prefer if we stayed under the column limit here. Done. https://codereview.chromium.org/12387018/diff/63003/gyp/tools.gyp#newcode197 gyp/tools.gyp:197: ['skia_win_debuggers_path and skia_os == "win" and skia_arch_width == 64', On 2013/03/08 16:30:56, borenet wrote: > Nit: and here Done. https://codereview.chromium.org/12387018/diff/63003/gyp/tools.gyp#newcode208 gyp/tools.gyp:208: ['skia_win_debuggers_path and skia_os == "win" and skia_arch_width == 32', On 2013/03/08 16:30:56, borenet wrote: > Nit: and here Done. https://codereview.chromium.org/12387018/diff/63003/tools/render_pdfs_main.cpp File tools/render_pdfs_main.cpp (right): https://codereview.chromium.org/12387018/diff/63003/tools/render_pdfs_main.cp... tools/render_pdfs_main.cpp:211: } On 2013/03/08 16:30:56, borenet wrote: > Nit: lines ending in spaces Done.
https://codereview.chromium.org/12387018/diff/63003/tools/win_dbghelp.cpp File tools/win_dbghelp.cpp (right): https://codereview.chromium.org/12387018/diff/63003/tools/win_dbghelp.cpp#new... tools/win_dbghelp.cpp:49: static void strncpyOrSetBlank(char* dest, const char* src, size_t len) { On 2013/03/08 16:44:25, edisonn wrote: > the problem is that someone might allocate dynamically the string in a new > version, and then mess up the strcpy. I prefer to keep it explicitly. Makes sense. Thanks for evaluating it. > But we could switch to using language D which has goodies like that AFAIK Let's leave that for a separate CL. :-)
https://codereview.chromium.org/12387018/diff/74001/gyp/tools.gyp File gyp/tools.gyp (right): https://codereview.chromium.org/12387018/diff/74001/gyp/tools.gyp#newcode198 gyp/tools.gyp:198: 'skia_arch_width == 64', Sorry for continuing to nitpick... can we make this a 4-space indent instead of 3? https://codereview.chromium.org/12387018/diff/74001/gyp/tools.gyp#newcode210 gyp/tools.gyp:210: 'skia_arch_width == 32', Same here.
https://codereview.chromium.org/12387018/diff/74001/gyp/tools.gyp File gyp/tools.gyp (right): https://codereview.chromium.org/12387018/diff/74001/gyp/tools.gyp#newcode198 gyp/tools.gyp:198: 'skia_arch_width == 64', On 2013/03/08 16:59:52, borenet wrote: > Sorry for continuing to nitpick... can we make this a 4-space indent instead of > 3? but it is a 4 space indent, I think it should be calculated from [ https://codereview.chromium.org/12387018/diff/74001/gyp/tools.gyp#newcode210 gyp/tools.gyp:210: 'skia_arch_width == 32', On 2013/03/08 16:59:52, borenet wrote: > Same here. same
https://codereview.chromium.org/12387018/diff/74001/gyp/tools.gyp File gyp/tools.gyp (right): https://codereview.chromium.org/12387018/diff/74001/gyp/tools.gyp#newcode198 gyp/tools.gyp:198: 'skia_arch_width == 64', On 2013/03/08 17:05:39, edisonn wrote: > On 2013/03/08 16:59:52, borenet wrote: > > Sorry for continuing to nitpick... can we make this a 4-space indent instead > of > > 3? > but it is a 4 space indent, I think it should be calculated from [ I disagree. To me, the proper alignment for a second line which is not indented further would be like this: ['blahblah......' 'blahblah'] Maybe the above is what we really want here anyway. Elliot, your thoughts?
https://codereview.chromium.org/12387018/diff/74001/gyp/tools.gyp File gyp/tools.gyp (right): https://codereview.chromium.org/12387018/diff/74001/gyp/tools.gyp#newcode198 gyp/tools.gyp:198: 'skia_arch_width == 64', done; yeah, look better like this On 2013/03/08 17:11:21, borenet wrote: > On 2013/03/08 17:05:39, edisonn wrote: > > On 2013/03/08 16:59:52, borenet wrote: > > > Sorry for continuing to nitpick... can we make this a 4-space indent instead > > of > > > 3? > > but it is a 4 space indent, I think it should be calculated from [ > > I disagree. To me, the proper alignment for a second line which is not indented > further would be like this: > > ['blahblah......' > 'blahblah'] > > Maybe the above is what we really want here anyway. Elliot, your thoughts?
I think you guys found the best way, and I think you'd better commit this NOW before we come up with any more nits to pick! On 2013/03/08 17:13:12, edisonn wrote: > https://codereview.chromium.org/12387018/diff/74001/gyp/tools.gyp > File gyp/tools.gyp (right): > > https://codereview.chromium.org/12387018/diff/74001/gyp/tools.gyp#newcode198 > gyp/tools.gyp:198: 'skia_arch_width == 64', > done; yeah, look better like this > On 2013/03/08 17:11:21, borenet wrote: > > On 2013/03/08 17:05:39, edisonn wrote: > > > On 2013/03/08 16:59:52, borenet wrote: > > > > Sorry for continuing to nitpick... can we make this a 4-space indent > instead > > > of > > > > 3? > > > but it is a 4 space indent, I think it should be calculated from [ > > > > I disagree. To me, the proper alignment for a second line which is not > indented > > further would be like this: > > > > ['blahblah......' > > 'blahblah'] > > > > Maybe the above is what we really want here anyway. Elliot, your thoughts?
Message was sent while issue was closed.
Committed patchset #27 manually as r8044 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
