In D131115#3707494, @thakis wrote:

Looks like this breaks tests on Windows: http://45.33.8.238/win/64016/step_11.txt

Please take a look, and revert for now if it takes a while to fix.

Fix test on Windows.

Use adjustAddr function declared in header.

Add explanatory comment for ra adjustment.

Correct enum naming convention.
Handle default PC type out of band.
Fix data test.

Change PC location information from angle brackets to brackets to avoid
conflicting with C++ template arguments.

Address review comments.

In D130251#3680141, @RKSimon wrote:

@mysterymath I've pushed rG1ea7b9c6ee6420dd6e87489534f44b92e1b6f220 as a tentative fix

This seems to be hitting assertions when compiling BoringSSL:
https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8807566603550542609/overview

Correct size directive.

Integrate from upstream.

Address review comments.

In D129519#3669903, @mysterymath wrote:

In D129519#3669837, @dyung wrote:

One of the tests you added symbolize-filter-markup-reset.test seems to be failing on the PS5 Windows bot, can you take a look?

https://lab.llvm.org/staging/#/builders/204/builds/1751

Looking on my Windows box; I'll revert if I can't get to the bottom of it in a few minutes.

In D129519#3669837, @dyung wrote:

One of the tests you added symbolize-filter-markup-reset.test seems to be failing on the PS5 Windows bot, can you take a look?

https://lab.llvm.org/staging/#/builders/204/builds/1751

In D129519#3669649, @mstorsjo wrote:

This broke building with GCC 9:

In file included from ../tools/llvm-symbolizer/llvm-symbolizer.cpp:23:
../include/llvm/DebugInfo/Symbolize/MarkupFilter.h:55:19: error: declaration of ‘const llvm::symbolize::MarkupFilter::Module* llvm::symbolize::MarkupFilter::MMap::Module’ changes meaning of ‘Module’ [-fpermissive]
   55 |     const Module *Module;
      |                   ^~~~~~
../include/llvm/DebugInfo/Symbolize/MarkupFilter.h:46:10: note: ‘Module’ declared here as ‘struct llvm::symbolize::MarkupFilter::Module’
   46 |   struct Module {
      |          ^~~~~~
../include/llvm/DebugInfo/Symbolize/MarkupFilter.h:65:19: error: declaration of ‘const llvm::symbolize::MarkupFilter::Module* llvm::symbolize::MarkupFilter::ModuleInfoLine::Module’ changes meaning of ‘Module’ [-fpermissive]
   65 |     const Module *Module;
      |                   ^~~~~~
../include/llvm/DebugInfo/Symbolize/MarkupFilter.h:46:10: note: ‘Module’ declared here as ‘struct llvm::symbolize::MarkupFilter::Module’
   46 |   struct Module {
      |          ^~~~~~

Address review comments.

Remove unnecessary flag from test RUN: lines.

Remove unused declaration.

Fix indentation.

I did a bit more hammering on the semantics of contexutal line elision.
Previously, contextual elements would be output as text if they were preceded
by text, which wasn't intended. Fixing this means that a non-contextual line
will be entirely deferred. This means that tryContextualLine() doesn't really
make sense anymore; it always handles the whole line. I've thus made the
modified version of it into the new body of filter().

Address review comments.

Address review comments. Refactored out printing module info line prefix.

Fixed use after free with MarginNodes. MarginNodes ended up being unnecessary,
since when a new module line is opened, the next thing to print will
necessarily be that module line. That means the next things to be output will
be the margin nodes, and these can be summarily printed.

preceeding -> preceding

Add periods. Correct typo.

Use Densemap<unique_ptr> for Modules; fix flipped overlappingMMap condition.

Clean up diff.

Accomodate Windows path separators in test.

Add dwarfv5 test and use 2>&1 for error checking.

Address review comments.

In D124686#3598029, @dblaikie wrote:

Is it already planned to add this markup to LLVM's own crash dumps when they aren't already symbolized? Might be a handy use-case for the feature, providing some built-in-to-llvm exercisizing/advertising/experience with the feature? (if the markup is meant to be compatible with human readers who might not be able to symbolize the data later - which seems like a nice feature too)

Apologies for the long delay; this change slipped my mind for a bit.

Address review comments.

In D126980#3604389, @peter.smith wrote:

Only a few small comments. May be worth calling Filter.cpp and Filter.h MarkupFilter.cpp and MarkupFilter.h as there could be non-markup based filters in the future. Not a strong opinion as these filenames could be changed later if needed.

I like MarkupFilter as a name; especially as a pair for MarkupNode and MarkupParser. I've renamed the files and classes to match.

Address review comments.

Extract deplibs before LTO, as discussed in #56070.

Fix TODO.

Rename coloring utils.

Integrate.

Missed comment.

Clean up usage of NextIdx.

Add missed NextIdx=0 call.

Refactor markup parser to parse lazily on each node request.

Add comments; remove assert.

Integrate.

Improve comment for flush().

Integrate.

Fix missed element->node comment update.

In D124686#3591327, @peter.smith wrote:

LGTM thanks for the updates. I spotted what looked like one comment update for the change to Node, but that can easily be fixed up if needed.

Address review comments.

I've created https://github.com/llvm/llvm-project/issues/56070 to track the general issue and provide more background on the issue's diagnosis.

In D127885#3587455, @MaskRay wrote:

This breaks the symbol resolution model of LTO. The symbol resolution intends to know all symbols and no new symbol can be added after LTO compiling.

Remove tryFilter abstraction; unneeded complexity.

Add pretty error location reporting.

Integrate.

Fix tests.

Integrate.

Integrate.

Fix parsing for empty field.

In D126815#3568630, @noajshu wrote:

I suppose another direction would be to abandon the producer-consumer model altogether. In D114845, this could be done (in part) by letting the threads share access to the recursive_directory_iterator, acquiring a lock then advancing it on their own to harvest jobs from the filesystem.

I like that; it should decouple those commits from this one, and it should be quite a bit cleaner than maintaining size counters.