Mark static member functions as static in CodeViewDebug
ClosedPublic

Authored by amccarth on Mon, Sep 11, 3:23 PM.

Details

Summary

To improve CodeView quality for static member functions, we need to make the
static explicit. In addition to a small change in LLVM's CodeViewDebug to
return the appropriate MethodKind, this requires a small change in Clang to
note the staticness in the debug info metadata.

Diff Detail

Repository
rL LLVM
amccarth created this revision.Mon, Sep 11, 3:23 PM
amccarth added a subscriber: llvm-commits.
rnk added a comment.Mon, Sep 11, 4:45 PM

This should have an IRGen test. You can probably extend clang/test/CodeGenCXX/debug-info-ms-abi.cpp with a static method.

clang/lib/CodeGen/CGDebugInfo.cpp
2907 ↗(On Diff #114717)

Please revert the unrelated whitespace change

3218 ↗(On Diff #114717)

ditto

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
327–329 ↗(On Diff #114717)

I suppose this is where we need to pass down a bool about this being a static member function.

1560–1564 ↗(On Diff #114717)

If we had a bool IsInstanceMethod or something here we could skip this drop_front() dance and that would fix our parameters.

1625 ↗(On Diff #114717)

I'd do this check first, since static methods can't be virtual.

amccarth marked 5 inline comments as done.Tue, Sep 12, 5:09 PM

Good tip on the lowering. Extra test coming momentarily.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1560–1564 ↗(On Diff #114717)

Ah yes, this fixes the Type field for static methods as well. I'll extend the test to show that.

1625 ↗(On Diff #114717)

And virtual methods can't be static, so I'm not sure the order really matters. But done because it's not worth a debate.

amccarth updated this revision to Diff 114945.Tue, Sep 12, 5:10 PM
amccarth marked 2 inline comments as done.

Adds code-gen test. Corrects problem with first param of static method being
treated as the type of the this pointer. Addresses style suggestions.

rnk accepted this revision.Tue, Sep 12, 5:32 PM

Looks good! Make sure to run check-clang and check-llvm. The CGDebugInfo.cpp change might affect clang codegen tests that generate debug info for DWARF.

This revision is now accepted and ready to land.Tue, Sep 12, 5:32 PM
In D37715#868973, @rnk wrote:

Looks good! Make sure to run check-clang and check-llvm. The CGDebugInfo.cpp change might affect clang codegen tests that generate debug info for DWARF.

So check-llvm is fine (except for one unrelated test that fails without my change). I've also used all the various combinations of check-*debuginfo*.

check-clang, however, gets hundreds of Python stack traces because:

TypeError: a bytes-like object is required, not 'str'

This sounded to me like a Python 2/3 problem, so I reset my environment to Python 2.7, re-ran CMake, and tried again. But I got the same set of Python errors. What's the trick here?

rnk added a comment.Wed, Sep 13, 11:35 AM

This sounded to me like a Python 2/3 problem, so I reset my environment to Python 2.7, re-ran CMake, and tried again. But I got the same set of Python errors. What's the trick here?

CMake discovers the python executable and caches it in CMakeCache.txt on first configuration, so changing the environment generally doesn't have an effect unless you re-configure with a clean CMakeCache.txt and CMakeFiles dir.

The quick way to get back to 2.7 is probably to edit CMakeCache.txt and change the PYTHON_EXECUTABLE variable to look something like this:

PYTHON_EXECUTABLE:FILEPATH=C:/Python27/python.exe

ninja check-clang will pick that up on the next run and use it to run the test suite.

Immediately issue aside, I guess it's worth setting up continuous testing with py3k. =/

This revision was automatically updated to reflect the committed changes.