If a user specifies multiple input files, should the tool symbolicate all of them rather than forcing user interaction to pick one?

It occurred to me that we'd need to do an in-place update of the existing file or drop and updated version of each file next to it. It also occurred to me that we don't have a specific file extension we can look for. Specifying multiple files with a wildcard would be problematic due to that.

@noahfalk / @shirhatti what are your thoughts on handling multiple files? Should we have users script around the tool to hit multiple files?

I think it would be better to receive only one specified extension(xxxxx.stack or xxxxx.stacktrace).
What do you think?

The issue with specifying an extension is that there isn't an accepted, standard extension for this type of output. This means we either need to have our users manually change the name of each file they want touched or specifically list all the files they want touched. In either case we are requiring the user to manually list out their files.

I'm inclined to not allow wildcards and only accept a list of specific files since it is simpler for our tool to handle. If a user wants to wildcard, they can rely on the shell's ability to do so, e.g., $ find ./*_exception_log | xargs dotnet-stack symbolicate.

We will have different default behaviors depending on the number of input files as a result:

• 1 input file --> print modified file to stdout
• >1 input file --> print modified file to new file next to old one (e.g., input.txt -> intput.sym.txt)

If a user specifies multiple input files, should the tool symbolicate all of them rather than forcing user interaction to pick one?

Should we allow multiple stacktrace files to be input? Isn't it better to input only one file at a time?
Or how about guiding the format of the input file contents?

  ------- Input file format -------
   at {typeName}.{methodName}({parameters}) in {moduleName}: token {methodToken}+{ilOffset}

@noahfalk / @shirhatti what are your thoughts on handling multiple files? Should we have users script around the tool to hit multiple files?

I would keep it simple (at least initially) and support a single file input and write the output to some path adjacent to the output. Then as optional improvements we could add arguments that change the output path or write the output to stdout. File output as the default appealed to me because:
a) It requires less shell scripting knowledge
b) Probably if the input started in a file the user wants the output in a file
c) If we later add support for multiple files the experience doesn't need to abruptly change behavior

Or how about guiding the format of the input file contents?

I assume we'll have to have some definition of the format the tool can parse. I'm not sure if I understood the question though.

I concur with @noahfalk. The tool should take one file in, and write to a new file next to the old one, e.g.,

$ ls
input.txt
$ dotnet stack symbolicate input.txt
$ ls
input.txt	input.symbolicated.txt

or whatever addition to the file extension we want to use to differentiate.

If you're asking whether we should require the input file to have a specific format, I'm not sure that's something we can require of users. There isn't a standardized file format that we can expect. I think it is sufficient to look for the at ... in ... token ... pattern and fixup when we find it.

There isn't a standardized file format that we can expect.

I think the format of the input file is already set in the function:
https://github.com/dotnet/runtime/blob/c93bb62e33934c3b8b6b1d293612d44360483bd8/src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackTrace.cs#L206
So, the user using this tool thought that it was easy to input without changing the output stack.
Because it is already output as below.
" at {typeName}.{methodName}({parameters}) in {moduleName}: token {methodToken}+{ilOffset}"

I think it is sufficient to look for the at ... in ... token ... pattern and fixup when we find it.

Also, I agree with you. But wouldn't most users use it without change? :)

I think we might be saying the same thing.

I think we should take in any file and treat it as a text file because there is no agreed upon file extension for stack traces like this. We should just scan the file for the pattern and attempt to fix it up in place, then save the fixed-up file next to the original.

I would expect we would only have one PDB per assembly. If you sorted both arrays before iterating and then used a for(;;;) loop to not lose your spot in the array, you would do less work. As it is now, this could potentially iterate all the assembly files for every PDB.

I would expect we would only have one PDB per assembly.

Right. But #2145 (comment)

In generally, CE products(mobile, wearable, TV, refrigerator, etc) do not have enough memory.
So, It is often deployed an application without a PDB file to reduce the deployment size or obfuscate.

I think PDB file per assembly may not exist. So, I generated XML file only when there is assembly and pdb of the same name. Do you have a better idea?

I was only suggesting a way to match up the assembly list and the pdb list with less looping.

for example (in pseudo code):

function match_assembly_pdb:
	pairList <- []
	pdbList.sort()
	asmList.sort()
	j <- 0
	for i in pdbList.length: # assuming pdbList.length < asmList.length
		while j < asmList.length:
			if pdbList[i] == asmList[j]:
				pairList.add((pdbList[i], asmList[j]))
				break
			j++

Then the code only ever touches each element of each array once after the sort.

All that being said, I chatted with my team a bit and I think there is a simpler way of getting the symbols without converting to XML. Rather than use DiaSymReader, you can use the System.Reflection.Metadata APIs directly since those already know how to manipulate portable PDBs.

Here is an example from SOS on how you can use these APIs:

Here is an example from the runtime on how to use those APIs (this one is a little simpler and handles caching parsed PDBs):
https://github.com/dotnet/runtime/blob/b1452c8fcab789dd9f2921c1479425b291111c5d/src/libraries/System.Diagnostics.StackTrace/src/System/Diagnostics/StackTraceSymbols.cs#L13-L94

Since non-portable PDBs are opt-in only and only work on Windows, I'm inclined to not add the extra headache of using DiaSym which ultimately wraps the System.Reflection.Metadata for portable PDBs anyway. This will help with caching parsed symbols, reduce dependencies, and decrease complexity by not requiring the conversion to XML phase.

I want to emphasize the use of System.Reflection.Metadata here since that should simplify the logic and dependencies.

I want to emphasize the use of System.Reflection.Metadata here since that should simplify the logic and dependencies.

Ok. I'll check the System.Reflection.Metadata

Can we cache this load and the symbol lookup? This looks like it's going to load the file, parse the XML, and walk every method in the parsed XML for every frame of a stack trace. If a stack has many frames from a single assembly with a large PDB, this could be expensive.

@hoyosjs are these packages managed under DARC or should we leave the manual versioning here?

I'd centralize them. Also, we already use Dia in SOS / dotnet dump.

FYI, in the latest versions of System.CommandLine, this property is not accessible.

I'd put these in a finally block, or refactor so you can use them in a using block.

This looks like it always writes the fixed up text to a .symbolicated file next to the input file. and then optionally also writes to an output file.

The tool should do one or the other, but not both since that would mean a user generates two files when they specify an output path.

I believe in a previous comment chain, we had decided to only print to a file. We can have it print to the console, but we should put it behind a --stdout flag or something similar.

This function seems to have a few side-effects unrelated to getting the regex. I'd rename the function to better reflect that. Something like TrySymbolicateLine or something similar, since it looks like it can fail.

In a similar vein, it looks like this function has the logic for writing to one file, while GetLineFromStack has the logic for writing to other outputs. Having the print logic spread out makes it difficult to reason about what's being written where and when. In this case, the function name doesn't imply that it does any writing.

Preferably, GetLineFromStack might look something like this:

outputWriter = outputPath.IsNullOrEmpty() ?
	/* open file next to input */ :
	/* open at output path */;
foreach (line in inputFile)
	outputWriter.Write(TrySymbolicateLine(line));

so all the writing happens in one place.

I don't think the name of this function reflects what it is doing. Perhaps something like SymbolicateFile?

Get implies this returns something, but it returns void.

nit: static readonly variables typically either get s_<name> or PascalCased names, e.g., s_tempDirectoryPath or TempDirectoryPath.

You can probably skip this stringification if you use DirectoryInfo[] and FileInfo[] everywhere.

The formatting for a single string is unnecessary work. This can just be Console.Out.WriteLine(ret).

This check should come before doing all the StringBuilder work above since you throw it out if it is false.

This does a full read of the line 3 times in a row. You could simply check against the compiled regex once that would either match an return all the necessary info or not match, all in one step.

It might be easier to pass the {File|Directory}Info[] around, so we don't have to stringify everything here.

If the order of entries in these arrays is used to locate files, the current directory should probably be at the front of the array.

nit: we can remove this function and just call CreateSymbolicateFile directly in Symbolicate.

This should attempt to cache the MetadataReaders so we don't repeat work if there are multiple frames from the same assembly.

I recommend stashing them in a Dictionary<string,MetadataReader> and being sure to clean them up at the end of the process.

When you add a caching mechanism for the MetadataReaders, you can eliminate this loop by making the key of the cache the assembly name. Then this becomes a constant time lookup rather than a linear lookup. You would need some function like bool TryGetMetadataReader(string assemblyName, out MetadataReader reader) that uses what you have written in GetMetadataReader that also attempts to cache things.

what does s_verifyRegex accomplish that s_regex doesn't already?