Re: [dbghelp 4/10] Rewrite much of the symbol lookup method to work with Optimized PDB files as well
Jason Green a écrit :
------------------------------------------------------------------------
From 14d91ebd5974c8fc02f8b83d53e8eff0df7ad76d Mon Sep 17 00:00:00 2001 From: Jason Green <jason(a)jave01.(none)> Date: Thu, 17 Jan 2008 16:41:11 -0500 Subject: [PATCH] Rewrite much of the symbol lookup method to work with Optimized PDB files as well. Also, improves a number of TRACEs, and wraps strings in the debugstr() functions.
what's the rationale of optimized pdb ? the patch is already big enough not to mix code evolution with stuffing new trace:s all over the place A+ -- Eric Pouech "The problem with designing something completely foolproof is to underestimate the ingenuity of a complete idiot." (Douglas Adams)
Eric, below are the responses from Eric van Beurden, who wrote the patch. I merely split it up and removed a bunch of traces for submission to WineHQ. The problem is that all of our changes were done initially in just a couple of huge commits during the initial import of dbghelp instead of nice, incremental changes. I split things up based mostly on a file by file basis, but I can further split this one as well On Jan 18, 2008 3:14 PM, Eric Pouech <eric.pouech(a)orange.fr> wrote:
what's the rationale of optimized pdb ?
An optimized PDB file is generated with an optimized build of an app. Symbol addresses and ranges can overlap in these files. The existing implementation neither takes that into account when sorting the list nor when looking up symbols. This results in symbol lookups failing ~85-90% of the time.
the patch is already big enough not to mix code evolution with stuffing new trace:s all over the place
Understood. Re: 7/10 - Fix file searching to search only listed directories instead of the whole HD:
you failed to set DebugFilePath to the desired value again the rest of the patch has nothing to do with the first part of the patch
Previously, the FindDebugInfoFile() function was using the function's return value string <DebugFilePath> as the me of the first file to open. This was an error since there is no requirement that this string be initialized to anything. However, you are correct in pointing out that the <DebugFilePath> variable did not get set in the event that the first attempt at finding the file succeeded. This fix will be submitted. Re: 10/10 - Clamp minidump memory blocks to 928KB:
if that's really the case (and I'm even not sure we want to support this backward compatibility), a good fix would be to add several streams each up to 928k instead of clamping
After much more research and testing this turned out to be in error. The issue was not actually the size of the single stream, it was the ordering of the streams that windbg did not like. Windbg expects the six standard streams in the following order or else it will have trouble opening some minidumps: SystemInfoStream MiscInfoStream ExceptionStream ThreadListStream ModuleListStream MemoryListStream The stream table at the start of the file is sorted in numerical order by stream ID (as the current implementation has), but the actual stream data must appear in the above order after the file header. Our current fix for this is rather hacky and needs to be fixed properly and cleaned up before resubmitting. The proper fix would be to change the order that each stream is created, then to sort the stream table by stream ID before writing it to file.
Jason Green a écrit :
Eric, below are the responses from Eric van Beurden, who wrote the patch. I merely split it up and removed a bunch of traces for submission to WineHQ. The problem is that all of our changes were done initially in just a couple of huge commits during the initial import of dbghelp instead of nice, incremental changes. I split things up based mostly on a file by file basis, but I can further split this one as well
On Jan 18, 2008 3:14 PM, Eric Pouech <eric.pouech(a)orange.fr> wrote:
what's the rationale of optimized pdb ?
An optimized PDB file is generated with an optimized build of an app. Symbol addresses and ranges can overlap in these files. The existing implementation neither takes that into account when sorting the list nor when looking up symbols. This results in symbol lookups failing ~85-90% of the time.
hmm I still don't get how, in a generic way symbols could overlap the only think I could come up with if when static functions get automatically inlined by the compiler, but that's rather a different story (as I'd suspect the inlined function to be a single memory range within a container => and in that case returning the shortest matching symbol should be fine is there an existing exe/pdb file available for further testings
the patch is already big enough not to mix code evolution with stuffing new trace:s all over the place
Understood.
[snip]
Re: 10/10 - Clamp minidump memory blocks to 928KB:
if that's really the case (and I'm even not sure we want to support this backward compatibility), a good fix would be to add several streams each up to 928k instead of clamping
After much more research and testing this turned out to be in error. The issue was not actually the size of the single stream, it was the ordering of the streams that windbg did not like. Windbg expects the six standard streams in the following order or else it will have trouble opening some minidumps: SystemInfoStream MiscInfoStream ExceptionStream ThreadListStream ModuleListStream MemoryListStream The stream table at the start of the file is sorted in numerical order by stream ID (as the current implementation has), but the actual stream data must appear in the above order after the file header. Our current fix for this is rather hacky and needs to be fixed properly and cleaned up before resubmitting. The proper fix would be to change the order that each stream is created, then to sort the stream table by stream ID before writing it to file.
hmm still not sure it's the correct explanation... the 928k value is not that clear (and doesn't explain the impact of the order) I'd rather fear something like windbg mmaps the first meg of minidump upfront, and expects to get most the relevant information (the 5 listed streams, potentially header and data, in that mapping)... memory is likely to be used differently by another memory mapping, hence getting around the 1meg barrier (that's still needs to be further tested anyway)
A+ -- Eric Pouech "The problem with designing something completely foolproof is to underestimate the ingenuity of a complete idiot." (Douglas Adams)
Re-responses from Eric van Beurden:
hmm I still don't get how, in a generic way symbols could overlap the only think I could come up with if when static functions get automatically inlined by the compiler, but that's rather a different story (as I'd suspect the inlined function to be a single memory range within a container => and in that case returning the shortest matching symbol should be fine is there an existing exe/pdb file available for further testings
in testing with one of our projects, i was seeing symbols lookups fail 85-90% of the time with the initial versions of dbghelp. The PDB files that were being used were generated along with the release build of the project (it was a C++ project as well). On further investigation i found that the lookups were failing because many symbol addresses and ranges were overlapping. This seemed to occur on even unrelated objects and symbols. I would suspect it was the result of the optimizations the compiler performed. I was able to generate PDBs that exhibited this behaviour with every test app i built under Visual Studio (v7.1 and 8.0Express); even apps that were only 25 lines long. Since it was very repeatable and the project in question would always be using PDBs generated from optimized release builds i decided to change the symbol lookup method to take the range into account and to be able to store a sublist of all symbols whose addresses fall into the same range (15-25% of the lookups still failed with just the range addition).
hmm still not sure it's the correct explanation... the 928k value is not that clear (and doesn't explain the impact of the order) I'd rather fear something like windbg mmaps the first meg of minidump upfront, and expects to get most the relevant information (the 5 listed streams, potentially header and data, in that mapping)... memory is likely to be used differently by another memory mapping, hence getting around the 1meg barrier (that's still needs to be further tested anyway)
Sorry, i don't think i explained it properly last time. The 928kb stream limiting change was incorrect and should be ignored now. It very well could be that windbg only mmaps the first 1MB of the file in an effort to find the SystemInfoStream information (and some others). The issue arose when we were performing a minidump that included a thread with an invalid ESP and EBP value in its Context struct. This resulted in us dumping the whole stack buffer (~1.14MB) for that thread instead of just the used area. Windbg refused to open the minidump after that. It complained that the SystemInfoStream could not be found (as it came after the MemoryListStream data in the file). The stream reordering was done for a couple of reasons: - to put the smaller, fixed size, standard streams first in the file where windbg seemed to want them - to better match the stream ordering that native dbghelp wrote its minidump files in. The stream ordering still isn't identical because native does things such as coalesce all strings into the same area of the minidump file (instead of putting each string immediately after the stream they belong to), and group stream data and non-stream data together.
participants (2)
-
Eric Pouech -
Jason Green