This is an archive of the discontinued LLVM Phabricator instance.

Fix template class debug info for Visual Studio visualizers
ClosedPublic

Authored by amccarth on May 1 2020, 4:04 PM.

Details

Summary

An earlier change eliminated spaces between the close brackets of nested
template lists. Unfortunately that prevents the Windows debuggers from
matching some types to their corresponding visualizers (e.g., std::map).

This selects the SeparateTemplateClosers flag when generating COFF debug
info. Note that we were already making formatting adjustments under
similar circumstances for similar reasons.

This solves the immediate problem, but there aren't many tests for COFF
debug info (that I can see). I will be looking into integrated tests with
Dexter next.

Diff Detail

Event Timeline

amccarth created this revision.May 1 2020, 4:04 PM

This should be testable with a -emit-llvm test FileChecking the generated IR?

rnk added a comment.May 4 2020, 1:26 PM

I think testing it is a matter of adding later C++ standard versions to the existing test here: ../clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp

amccarth updated this revision to Diff 262499.May 6 2020, 4:46 PM

Updated an existing test. Thanks for the pointer; I had gotten myself completely confused about the organization of the test directories.

Updated test to validate the change.

rnk added inline comments.May 6 2020, 6:00 PM
clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp
10 ↗(On Diff #262499)

Why choose fms-compatibility? Does it have a side effect of raising the default standard version?

12 ↗(On Diff #262499)

Any reason not to reuse --check-prefix=UNQUAL? This should be the same as the first RUN line, with a different standard.

amccarth marked 2 inline comments as done.May 7 2020, 8:51 AM
amccarth added inline comments.
clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp
10 ↗(On Diff #262499)

Why choose fms-compatibility?

I thought that -fms-compatibility was the solution we had discussed.

With ms-compat, we retain the space. Without ms-compat, clang does what clang does (which, currently, is to omit the space).

Does it have a side effect of raising the default standard version?

That was not my intent. Removing the -std=c++98 raises the standard.

12 ↗(On Diff #262499)

This partly overlaps with the thread about ms-compat. But apart from that, I was trying to be explicit about what's being tested, which, in this case, is the ms-compat. The reason the old test got the space is somewhat tangential to the qualified/unqualified distinction.

If you wanted to not pin to C++98 and not use ms-compat, then you wouldn't expect the space. Maybe that's something that I should also test.

rnk added inline comments.May 11 2020, 1:31 PM
clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp
10 ↗(On Diff #262499)

From looking at the code, it seems like we omit the space in debug info when codeview is enabled, regardless of -fms-compatibility or any other setting. The flag for emitting the space is conditional on the standard version, so I think what we want here is re-run of the same test with a higher standard version, something beyond C++11.

I guess, responding to your comment below, the idea is that this test uses the default C++ version, whatever it is. I guess my feedback is, let's test all of the following:

  1. explicit C++98
  2. explicit C++11
  3. unpinned std
amccarth updated this revision to Diff 263790.May 13 2020, 10:56 AM

Addressed feedback, specifically:

  • Distinction is now on CodeView generation rather than -fms-compatibility.
  • Tests two --std= levels plus the default one.
amccarth marked 2 inline comments as done.May 13 2020, 10:57 AM

Made the requested changes after an in-person conversation to clear up my earlier confusion.

rnk accepted this revision.May 13 2020, 12:43 PM

lgtm, thanks!

This revision is now accepted and ready to land.May 13 2020, 12:43 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2020, 2:43 PM