Page MenuHomePhabricator

RFC: Remove "Validators"
ClosedPublic

Authored by aprantl on Dec 10 2019, 3:37 PM.

Details

Summary

Data formatter Validators are completely wired up all the way to the SBAPI, but as far as I can tell they are not used anywhere (including swift-lldb) and also are not tested. I'm puzzled by this, so if anyone can share some info about what this was intended for, please let me know.

This patch deletes them as dead code.

Diff Detail

Event Timeline

aprantl created this revision.Dec 10 2019, 3:37 PM

Adrian and I talked about this some more. Apparently the idea was that you have some type Foo and you want to look for some error state in instances of that type (Foo::a + Foo::b < 10). So you add a Type Validator for Foo that does this check, and every time lldb prints a variable of type Foo, it will run the Validator on it, and if it fails validation, then the printer will print an ! at the beginning of the printing, and also there's an SB API to get whether the Value passed the validator.

So then you could just debug along, and either look for the ! in the printing, or add a stop hook that checks the Validator result on all locals, and if you ever saw the error state, you would know to investigate further. That's actually a pretty neat idea.

The current state of the code is that there is actually no way to add a Type Validator. To be really useful, there would need to be a way to create a scripted validator, so the Python bindings and some command/SB API to register the validator.

The implementation is a little cut-and-paste too. It shares all the same options with the Synthetic child provider & Summaries (skips pointers, cascade, etc.) but I don't think you would ever want a type validator that would only validate references to a type, but not the type itself. And then the implementation is very cut & paste. So I'm fine with deleting this for now, but maybe adding it as an interesting project idea to the Projects page - with a reference to the hash of this commit as a starting point?

It's a neat idea but it's also been 5 years now and it was never made useful...

davide added a subscriber: davide.Dec 10 2019, 4:08 PM

Adrian and I talked about this some more. Apparently the idea was that you have some type Foo and you want to look for some error state in instances of that type (Foo::a + Foo::b < 10). So you add a Type Validator for Foo that does this check, and every time lldb prints a variable of type Foo, it will run the Validator on it, and if it fails validation, then the printer will print an ! at the beginning of the printing, and also there's an SB API to get whether the Value passed the validator.

So then you could just debug along, and either look for the ! in the printing, or add a stop hook that checks the Validator result on all locals, and if you ever saw the error state, you would know to investigate further. That's actually a pretty neat idea.

The current state of the code is that there is actually no way to add a Type Validator. To be really useful, there would need to be a way to create a scripted validator, so the Python bindings and some command/SB API to register the validator.

The implementation is a little cut-and-paste too. It shares all the same options with the Synthetic child provider & Summaries (skips pointers, cascade, etc.) but I don't think you would ever want a type validator that would only validate references to a type, but not the type itself. And then the implementation is very cut & paste. So I'm fine with deleting this for now, but maybe adding it as an interesting project idea to the Projects page - with a reference to the hash of this commit as a starting point?

It's a neat idea but it's also been 5 years now and it was never made useful...

+1. This code can be resurrected as-needed.

aprantl accepted this revision.Dec 11 2019, 8:37 AM

That sounds like there is consensus.

This revision is now accepted and ready to land.Dec 11 2019, 8:37 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptDec 11 2019, 9:28 AM

Would you mind adding a mention on the Projects page? This was a neat idea and realistically if it just sits in git, in a few months no one will remember it's there.

Would you mind adding a mention on the Projects page? This was a neat idea and realistically if it just sits in git, in a few months no one will remember it's there.

Done.

vsk added a subscriber: vsk.Dec 12 2019, 10:56 AM

Just a heads up while I investigate, I'm starting to see this on the sanitizer bot:

/Users/buildslave/jenkins/workspace/lldb-cmake-sanitized/llvm-project/lldb/include/lldb/DataFormatters/TypeCategory.h:278:35: runtime error: load of value 190, which is not a valid value for type 'bool'

http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake-sanitized/621

This patch is just deleting dead code so it's probably unrelated.

vsk added a comment.EditedDec 12 2019, 11:05 AM

I believe this change is responsible:

-      m_validator_cont("validator", "regex-validator", clist), m_enabled(false),

@aprantl was the m_enabled initialization supposed to be deleted?

Edit: I went ahead and put the initialization back in 46d970cc436, let me know if some other change is needed.

Whoops. Thanks for fixing it!