Page MenuHomePhabricator

[NativePDB] Add anonymous namespaces support

Authored by aleksandr.urakov on Apr 17 2019, 5:19 AM.



This patch adds anonymous namespaces support to the native PDB plugin.

I had to reference from the main function variables of the types that are inside of the anonymous namespace to include them in debug info. Without the references they are not included. I think it's because they are static, then are visible only in the current translation unit, so they are not needed without any references to them.

There is also the problem case with variables of types that are nested in template structs. For now I've left FIXME in the test because this case is not related to the change.

Diff Detail


Event Timeline

Don't know much about PDBs, but the change seems pretty straight-forward to me. Zach would be the best person to review this, but he's probably busy with other things now. Maybe Adrian could take a look?

Sorry for the slow response; I'm still learning about this code.

I like where this is going.

77 ↗(On Diff #195540)

Is this a new problem because of your change? Or is this an already existing problem you discovered in the process? Is the cause understood? Should you add a commented-out CHECK showing what the test would be when this problem is fixed?

575 ↗(On Diff #195540)

Again, I think you should drop the .c_str().

812 ↗(On Diff #195540)

Normally, I would agree that StringRef is the right type for name, but looking at the three call sites, it seems that this is going to cause a lot of unnecessary conversions.

The call sites are all passing std::string::c_str(), which is a const char *. So we're constructing a StringRef name (which involves a strlen). The name is then passed on as name.str().c_str() which constructs a new std::string only to pass a const char * again.

I'd argue that, in this case, we're better off declaring name as a const char *.

813 ↗(On Diff #195540)

I'd also recommend making GetOrCreateNamespaceDecl private, since the only callers are here in PdbAstBuilder.cpp.

amccarth marked an inline comment as done.Apr 18 2019, 1:39 PM
amccarth added inline comments.
575 ↗(On Diff #195540)

Disregard this comment. I was going to make a different suggestion and this was a leftover part of that.

aleksandr.urakov marked an inline comment as done.Apr 18 2019, 11:54 PM
aleksandr.urakov added inline comments.
77 ↗(On Diff #195540)

It's an existing problem, but I didn't go deep into it yet. I've added the test lines describing how it should be, thanks.

Thank you for the comments, I agree with you. I've fixed the patch.

amccarth accepted this revision.Apr 19 2019, 8:17 AM

Thanks for the changes. If the tests pass, then this LGTM. Just check the one last question I added about the AC1D test.

(The diff in Phabricator looks very confused, as though you deleted several hundred lines. Perhaps because the original diff had context and this one didn't. If I'm interpretting the diff correctly, this looks good now.)

22 ↗(On Diff #195863)

Should this one start with FIXME rather than CHECK? Isn't this the case that's currently broken?

This revision is now accepted and ready to land.Apr 19 2019, 8:17 AM

Thanks for the review! Sorry, I've forgot to include context in the updated patch. Yes, it's exactly the case that is broken, I've left FIXME there until it will be fixed.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2019, 12:12 AM