484837 - Use abc file name for dmp name rather than generic avmplusCrash.dmp

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Rick Reitmaier]

Make the dmp files more useful by identifying which input caused the crash.  This will be handy when running the acceptance suite since it continues running despite crashes and may cause dmp's to be overwritten.
 
Would be nice to have something similar for osx and unix but that's for another day.

Comment 1

[Rick Reitmaier]

Attachment: ver 1 - patch

Comment 2

[Lars T Hansen]

Comment on attachment 369008 [details] [diff] [review]
ver 1 - patch 

I believe WinPlatform::processingFile will crash for filenames of the form "a/b.c/e".  Possibly the last test wants to check bwd>nm as well.

(Also need implementations for other platforms, obviously.)

Comment 3

[Edwin Smith]

Comment on attachment 369008 [details] [diff] [review]
ver 1 - patch 

avmshellWin.cpp:241  -  do we have VMPI_strcat?  that's preferred if we have it.

what happens if we aren't processing a file (eval repl mode, or maybe reading stdin if that's possible) and the shell crashes; do we have a reasonable default dump filename?

(marking + because i didn't find any additional issues, but with >1 reviewier, imo it's not a voting process, need unanomous + to commit)

Comment 4

[Lars T Hansen]

BTW the code is not standards conforming.  You're not allowed to compare pointers with relational operators unless they both point within the same object, yet strrchr can return NULL.  The comparisons bwd>nm and fwd>nm are therefore both illegal in this strict sense.

Comment 5

[Steven Johnson]

(In reply to comment #3)
> (From update of attachment 369008 [details] [diff] [review])
> avmshellWin.cpp:241  -  do we have VMPI_strcat?  that's preferred if we have
> it.

just a thought here: since we're forbidding standard c string operations and using VMPI_ based ones, perhaps this is an opportunity to move to the "safe" versions (ie the ones that always require buffer-size as an argument)? 

yes, it would be a bit more work, since we'd have to provide wrappers for systems that don't provide them intrinsically (ie, everything except VC2008).

Comment 6

[Rick Reitmaier]

#2 good catch; other platforms won't support the dmp file, so stubs are used.
#3 strncpy does not guarantee a null termination so strcat shouldn't be used
#4 the comparisons occur post-null check in the compound if statement, so they are safe.  But your point is well noted and it probably highlights that the code is awkward and needs to be re-written.
#5 I've used strncpy which limits the copy, and strcpy source is from a non-constant null terminated string.  I don't like using cat, since it implies that the destination is null-terminated which may not be the case.

Will post another patch.

Comment 7

[Rick Reitmaier]

Attachment: ver 2 - updated

Addressed issues raised in comments

Comment 8

[Rick Reitmaier]

re: comment #3, noticed that patch is missing initialization of dmpFilename in main() : 

@@ -264,6 +296,7 @@ avmshell::Platform* avmshell::Platform::
 
 	int main(int argc, char *argv[])
 	{
+        VMPI_strcpy(dmpFilename, "tamarinCrash.dmp");
 		SetErrorMode(SEM_NOGPFAULTERRORBOX);
 		
 		avmshell::WinPlatform platformInstance;

Comment 9

[Lars T Hansen]

Looks like it's ready to land, though.

Comment 10

[Dan Smith]

Still ready to land.

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A v3

(this is just a rebase of ver 2.  I haven't tested that it even builds, so I'm not yet marking attachment 369107 [details] [diff] [review] as obsolete; but if it works, then one should mark attachment 369107 [details] [diff] [review] as obsolete.)

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.