Vladislav Vaintroub skrev:
> Hi,
>
> Thank you very much for the quick review!
>
> I applied most of your suggestions, here http://lists.mysql.com/commits/42461
> See below for explanations.
Hi, just rechecked latest patch and it looks very good too me. Also
happy with your explanations to my comments. ;) It's always interesting
to learn new things.
Applied the patch to my tree and added a crash in 'mysql_update' and ran
"./mysql-test-run.pl alias" -> the mysqld crashed and generated a
minidump. I could then just double click that file and it would open in
the debugger and show me the correct location. Perfect.
I could however not find the output from stacktrace, but I don't think
you in general can see the output frome mysql when run from our test
suite. I might have fixed that in the new version of mysql-test-run.pl
I'm working on, will double check.
Also would like to do some manual testing, starting the mysqld from
console. Haven't got aroun that yet.
See answers to your answers below:
>
>
>> 1. there is one "hack" for BUG#18235 that can now be removed.
>>
>> mysqld.cc>>
>> #ifdef __WIN__
>> sig != SIGINT && /* Bug#18235 */
>> #endif
>
> Removed
>
>> 2. There are some unused functions in mysqld.cc
>> handle_kill, utsname and uname.
>
> Removed
>
>> 3. There are code to initialize...
>
> Maybe removed, don't known which places you meant exactly. I did not touch my_init.c
>
>
> sql/CMakeLists.txt
>
>> magnus: Why add header file (stacktrace.h)? No other .h files are in the list.
> This is really only for use in the IDE. *.h files go to Header files section and can
> be searched with "Find in Files" (entire solution or project).
> I also encounter 5 other header files sql/CMakeLists.txt
> hash_filo.h
> ${PROJECT_SOURCE_DIR}/sql/sql_yacc.h
> ${PROJECT_SOURCE_DIR}/include/mysqld_error.h
> ${PROJECT_SOURCE_DIR}/include/mysqld_ername.h
> ${PROJECT_SOURCE_DIR}/include/sql_state.h
Aha, so the header files has to be listed as part of the project for
them to be searchable with "Find in files". Well, of course that gives
advantage. Usually i noticed that the "Go to declaration" and "Go to
definition" anyway. There also seems to be more than the above mentioned
.h files in the mysqld project(referenced in other CMake rules)
Maybe we could write a CMake rule to auto add header files?
>
>
> sql/mysqld.cc
>> magnus: Add "#ifdef DEBUG_UNHANDLED_EXCEPTION_FILTER" aruond
>> 'wait_for_debugger'
> Done. Note, that this function is potentially interesting for other things than just
> debugging exception filter.
> I often wish there were an option to freeze mysqld when starting the test suite until
> debugger is attached (--gdb is not going to work on windows).
> Also, debugging mysqld startup when running as service would probably require such
> functionality.
Ok, you keep hat name for now then.
You can auto attach the debugger to mysqld by using
--debugger=vcexpress|vc|devenv when running a test from mysql-test-run.pl
>
>
>>> + __except(EXCEPTION_EXECUTE_HANDLER)
>>> + {
>>> + const char msg[] = "Got exception in exception handler!\n";
>>> + DWORD written;
>>> + WriteConsole(GetStdHandle(STD_OUTPUT_HANDLE),msg, sizeof(msg)-1,
>>> + &written, NULL);
>> magnus: how come you need to use WriteConsole here? Wouldn't
>> fprintf(stderr be same? And what happens if there is no console?
>
> WriteConsole writes nothing , if there is not console. Good you've seen this. I
> changed is to WriteFile now.
> So why WriteFile() and why fprintf or write() here is not appropriate?
> fprintf would be the same under normal circumstances, but the fact that server now
> crashed in crash handler might be well due to corrupt heap and hosed C runtime structures,
> stderr for example. So by now we already have 2 exceptions (original one and one in the
> handler) and fprintf will add the third to the exception chain, which is not nice.
> WriteFile is lot harder to crash, it does not involve heap at all as very thin syscall
> wrapper.
Interesting. ;)
>
>>> +#ifndef __WIN__
>>> + /* On Windows, do not terminate, but pass control to exception
>> filter */
>>> exit(1);
>>> +#endif
>> magnus: Alternative?
>> #ifdef __WIN__
>> return;
>> #endif
>
> The alternative gives "statement not reached"-warning on the exit() line (compiled
> with warning level /W4)
ok
>
>
>>> + /* Avoid MessageBox()es*/
>>> + _CrtSetReportMode(_CRT_WARN, _CRTDBG_MODE_FILE);
>>> + _CrtSetReportFile(_CRT_WARN, _CRTDBG_FILE_STDERR);
>>> + _CrtSetReportMode(_CRT_ERROR, _CRTDBG_MODE_FILE);
>>> + _CrtSetReportFile(_CRT_ERROR, _CRTDBG_FILE_STDERR);
>>> + _CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_FILE);
>>> + _CrtSetReportFile(_CRT_ASSERT, _CRTDBG_FILE_STDERR);
>> magnus: Please check in mysys/my_init.c, there is some code that does
>> the same.
>
> Yes, there is such code (currently inactive because we always use SAFEMALLOC in debug
> mode)
> But is for different purpose - avoiding CRT popups while reporting memory leaks and
> at process termination and for any process not just mysqld. I left it as it is for now.
ok, just so you were aware of it.
>
>>> +#ifndef __WIN__
>> magnus: I'm quite suire you need to include my_global.h, to get correct
>> settings for our defines.
>
> Corrected. Unfortunately, the correction required a hacky workaround for Bug#32082
> (definition of VOID in my_global.h conflict with windows headers)
> It worked BTW quite well without my_global.h defines (i.e with bare Win32 and stdlib
> stdio), but I agree #include for the exported functions is a cleaner solution.
Thats' ugly. Please recommit your patch for 6.0 tree and I'll help
review it. Removing our "#define VOID (X) __void__ = (int) (X)" looks
like a good idea.
>
>
> Stacktrace.c
>> + if(!GetCurrentDirectory(sizeof(path), path))
>> + return;
> magnus: Comment says "temp dir"? But this is current dir.
>
> Changed comment (look, the comment became historical/obsolete even before the first
> commit!)
yes, and you now removed that call completely. Fine. That is just how
the core file is written.
>
>>> + if(!GetModuleFileName(NULL, path2, sizeof(path2)))
>>> + return;
>>> + if(!(p= strrchr(path2,'\\')))
>>> + return;
>> magnus: Not sure I like the "error handling" ie. just return.
>
>
> Hmm, I'd really like to see a test case where one of these functions does not work,
> provided, we have MAX_PATH buffer for the full executable filename
>
> GetModuleFileName - does not do much, it takes executable name from the loaded image
> header and copies it to user supplied buffer.
> strrchr - not having backslash in full executable path in native format on Windows
> would wonder me much.
> Given that the probability to fail here is about 0%, the return as error handling is
> OK in my opinion. Alternatives?
> Assert in crash handler code has a WTF effect. Not checking at all makes static code
> analyzers unhappy. Trying a fallback, if I do not know how
> to reproduce the error is silly.
ok, that seems alright.
>
>> magnus: Would recommend using MySQL's string functions for string
>> manipulation.
> I've changed this place in the recent commit, removed own path parsing in favor of
> _splitpath(), which reduced total number of string functions to exactly one (strncat,
> sorry do not know MySQL equivalent ;))
The function you are looking for is 'fn_format' It can remove/replace
path, replace/remove extension all in one call.
It allows you to redeclare "path" as:
char path[FN_REFLEN];
and remove the "dump_fname" buffer.
Then call it like this after you have called GetModuleFileName.
fn_format(path, path, "", ".dmp", MYF(MY_REPLACE_EXT));
Also the usage of "_fullpath" to generate a full path of where the file
was written to won't actually work. You prepend path to dump_fname, but
path is pointing to the location of mysqld binary. Mysqld will run with
current dir in it's datadir. I suppose you need to use some other
function/or look at some global variable to extract that info.
It's very useful to know where the dump ended up so please figure out a
way to do it.
/ Magnus
