Page MenuHomePhabricator

[lldb][TypeSystemClang] Desugar an elaborated type before checking if it's a typedef or getting a typedefed type
ClosedPublic

Authored by aleksandr.urakov on Apr 23 2020, 12:07 AM.

Details

Summary

Sometimes a result variable of some expression can be presented as an elaborated type. In this case the methods IsTypedefType() and GetTypedefedType() of SBType didn't work. This patch fixes that.

I didn't find the test for these API methods, so I added a basic test for this too.

Diff Detail

Event Timeline

labath added a subscriber: labath.Apr 23 2020, 12:41 AM
labath added inline comments.
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
547

The name of this function is fairly misleading as it only desugars elaborated types (but not e.g. auto, decltypes, typeof expressions, etc).

Do you want to desugar those too? If yes, then you could call RemoveWrappingTypes while passing "typedef" as the thing-to-avoid.

Hello, Pavel! Thanks for the review. Yes, this method looks like a better fit, I just didn't notice it before.

LGTM now beside some minor request for the test. Thanks for the patch!

lldb/test/API/lang/cpp/typedef/TestCppTypedef.py
34

I know we do this (sadly) really often in LLDB, but these static error messages are just not useful. If my expression fails and the error message is "expression failed" then that doesn't help me with debugging the issue (especially when it's the only thing some remote bot sends back to me after a commit).

You could do self.assertTrue(expr_result.IsValid(), "Expression failed with:" + expr_result.GetError().GetCString()) instead and then people see the actual compiler output in the error log. Same for the other expr evaluation below.

labath added inline comments.Apr 24 2020, 4:04 AM
lldb/test/API/lang/cpp/typedef/TestCppTypedef.py
34

One of these days, I'm going to write assertSuccess/assertFailure functions which know how to test&print SBError objects. But I'm fairly busy these days, so if someone wants to beat me to it, be my guest.

Thanks! Fixed. The only thing is that GetCString() can return nullptr, which leads to None in Python, so I made a simple wrapper for that.

I'm not very deep into the testing infrastructure, so I'm not sure that I am a right person to implement assertSuccess etc. Sorry about that.

Thanks! Fixed. The only thing is that GetCString() can return nullptr, which leads to None in Python, so I made a simple wrapper for that.

I think GetCString should always return something when the expression fails unless something really goes wrong. I think the wrapper is a good idea for a more generic error handling code but in this case it just makes the test verbose and doesn't really add any new information (with and without the wrapper the program aborts with the same backtrace from what I can see. I would even say the concatenation with None error is a bit clearer in this situation, as we have a failure without an error string). Otherwise this is good to go.

I'm not very deep into the testing infrastructure, so I'm not sure that I am a right person to implement assertSuccess etc. Sorry about that.

Sure, I don't think that was meant to be a suggested enhancement for this specific line of patches.

The problem here is that in the case of success GetCString() returns nullptr, and we fail on concatenation with None even if the expression was evaluated successfully.

teemperor accepted this revision.Apr 24 2020, 5:58 AM

The problem here is that in the case of success GetCString() returns nullptr, and we fail on concatenation with None even if the expression was evaluated successfully.

Oh true, totally missed that. I think we usually solved that by putting the assert in some 'if', but that's not much better than the wrapper. And this hopefully anyway goes away with the discussed utility method. So let's just leave it at that for now and no longer delay this patch.

LGTM, thanks for the quick turnaround!

This revision is now accepted and ready to land.Apr 24 2020, 5:58 AM

The problem here is that in the case of success GetCString() returns nullptr, and we fail on concatenation with None even if the expression was evaluated successfully.

You should be able to use str(sb_error)

I'm not very deep into the testing infrastructure, so I'm not sure that I am a right person to implement assertSuccess etc. Sorry about that.

Sure, I don't think that was meant to be a suggested enhancement for this specific line of patches.

yep.

Exactly! Removed unneeded wrapper.

This revision was automatically updated to reflect the committed changes.