Page MenuHomePhabricator

Rework breakpoint language filtering to use the symbol context's language.
ClosedPublic

Authored by dawn on Dec 7 2015, 11:46 PM.

Details

Summary

This patch reworks the breakpoint filter-by-language patch to use the symbol context instead of trying to guess the language solely from the symbol's name. This has the advantage that symbols compiled with debug info will have their actual language known. Symbols without debug info will still do the same "guess"ing because Symbol::GetLanguage() is implemented using Mangled::GuessLanguage(). The recognition of ObjC names was merged into Mangled::GuessLanguage.

Diff Detail

Repository
rL LLVM

Event Timeline

dawn updated this revision to Diff 42142.Dec 7 2015, 11:46 PM
dawn retitled this revision from to Rework breakpoint language filtering to use the symbol context's language..
dawn updated this object.
dawn added a reviewer: jingham.
dawn set the repository for this revision to rL LLVM.
dawn added a subscriber: lldb-commits.
jingham requested changes to this revision.Dec 8 2015, 12:01 PM
jingham edited edge metadata.

The part of this fix which is using info in the SymbolContext to make the language detection more accurate is fine.

We have to do something better about how we detect the language from a mangled name. The target knows some that it should be able to contribute, but there's no clear way to do that now. But you aren't making that any worse (except the one inline comment about cstring_is_mangled above, which will need to get fixed) so I don't think you are obligated to solve that problem now.

This revision now requires changes to proceed.Dec 8 2015, 12:01 PM
dawn added a comment.Dec 8 2015, 3:28 PM

The part of this fix which is using info in the SymbolContext to make the language detection more accurate is fine.

We have to do something better about how we detect the language from a mangled name. The target knows some that it should be able to contribute, but there's no clear way to do that now. But you aren't making that any worse (except the one inline comment about cstring_is_mangled above, which will need to get fixed) so I don't think you are obligated to solve that problem now.

Can we please accept this as is? It helps *a lot* - we know the *exact* language whenever we have debug info for an identifier. I've been able to remove my #if 0's around the filtering code with this patch. As you say, it doesn't make anything worse, and for many of us makes, things are far better. Why would you reject it?

The name mangling issue is a whole other beast - it was a flawed assumption from the start (and I complained about it then). I'm simply trying to make the best with what we have.

(Added Greg - he wrote Mangled::GetLanguage - now GuessLanguage).

This patch removes the dependence on determining language from the name mangling for 99% of cases (there's still the much less common problem for symbols since Symbol 's GetLanguage calls Mangled::GuessLanguage). It is a win win.

dawn requested a review of this revision.Dec 8 2015, 3:39 PM
dawn edited edge metadata.

Please reconsider. Thanks.

clayborg accepted this revision.Dec 8 2015, 3:45 PM
clayborg edited edge metadata.

Just fix the comment as mentioned in the inline comments and this is ready. Jim should OK this as well.

include/lldb/Target/Language.h
176 ↗(On Diff #42142)

Fix comment to say "so if LanguageIsC(language)" since the variable below is "language".

dawn marked an inline comment as done.Dec 8 2015, 5:25 PM

Thanks Greg. Will fix comment in commit.

@Jim, can you accept please?

"cstring_is_mangled" does NOT mean that the string is a C++ mangled string, it means it is ANY kind of mangled string - you can tell because it is used to determine whether to populate the m_mangled and m_demangled components of the Mangled structure, which it should do regardless of the language the compiler happens to be mangling . If you look at the Github sources with Swift support, you will see that this returns true for both C++ mangled names and Swift mangled names, as it should.

It looks like you are using a function that had this error originally, so it is not your fault per se. We were probably getting away with it because it was never used in a case where its result mattered (it isn't used much...) But you are now using it somewhere where the incorrect usage would matter. So it needs to be fixed before the patch is correct.

For the nonce, it is probably good enough for Mangled::GuessLanguage to return C++ if the mangling scheme is Itanium or MSVC. We definitely have to figure out a better way to this, so a comment to that effect would also be in order.

This patch is not acceptable as is. It enshrines an incorrect usage of the function cstring_is_mangled which we shouldn’t do. As I said in the previous comment, it’s okay to come up with a short term fix, and check the mangled flavor against Itanium and MSVC. So it shouldn’t be hard to replace the errant usage. Do that and its fine to go in.

Jim

dawn updated this revision to Diff 42911.Dec 15 2015, 2:28 PM
dawn edited edge metadata.

Updated patch which changes Mangled::GuessLanguage to use the same algorithm for detecting the language from the mangled name as was added to LanguageRuntime::GuessLanguageForSymbolByName.

Jim, sorry for the previous ping - I missed your comment which included your objections. Hopefully the patch is acceptable now?

jingham accepted this revision.Dec 15 2015, 2:31 PM
jingham edited edge metadata.

That's great.

This revision is now accepted and ready to land.Dec 15 2015, 2:31 PM
This revision was automatically updated to reflect the committed changes.