This is an archive of the discontinued LLVM Phabricator instance.

Hide runtime support values such as clang's __vla_expr from frame variable
ClosedPublic

Authored by aprantl on May 2 2019, 10:10 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

aprantl created this revision.May 2 2019, 10:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2019, 10:10 AM

Could you add a comment (probably best in LanguageRuntime.h) saying what a RuntimeSupportValue is?

Also, so far as I can tell, this patch works because we use the CPP language runtime for C++ methods is an ObjC++ file, and the ObjCLanguageRuntime for ObjC methods in ditto. That is surely the right thing to do, but it would be good to ensure that there's a test that asserts this behavior. I'm sure there must be but didn't see on a quick scan. Can you make sure there's such tests and if not add one?

Could you add a comment (probably best in LanguageRuntime.h) saying what a RuntimeSupportValue is?

Done.

Also, so far as I can tell, this patch works because we use the CPP language runtime for C++ methods is an ObjC++ file, and the ObjCLanguageRuntime for ObjC methods in ditto. That is surely the right thing to do, but it would be good to ensure that there's a test that asserts this behavior. I'm sure there must be but didn't see on a quick scan. Can you make sure there's such tests and if not add one?

I'm not sure what exactly you want me to test there. Can you elaborate?

aprantl updated this revision to Diff 197843.May 2 2019, 12:22 PM
aprantl updated this revision to Diff 197871.May 2 2019, 2:39 PM

Thanks you! There was indeed a bug that prevented us from recognizing the correct runtime for the C++ this pointer.

jingham accepted this revision.May 2 2019, 2:50 PM

LGTM

This revision is now accepted and ready to land.May 2 2019, 2:50 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2019, 4:07 PM
labath added a subscriber: labath.May 3 2019, 1:23 AM

I think there is a problem with this patch. I don't believe the TestVLA part would work on any non-darwin platform (it doesn't on linux: http://lab.llvm.org:8014/builders/lldb-x86_64-debian/builds/923), and it works on darwin only accidentally.

The reason the fails on linux is that we don't have a "language runtime" plugin for the "C" language, so there is noone to ask if the variable is a runtime support value, and so we return false from ValueObject::IsRuntimeSupportValue. I believe the reason this works on darwin is because ValueObject::IsRuntimeSupportValue has a fallback (not sure why) which explicitly asks for the ObjC runtime if the desired language is not present. On linux, we don't have an ObjC runtime..

I can think of a couple of solutions here (adding a C language runtime, defaulting to return IsArtificial in ValueObject if no language runtime is present) but none of them seemed too obvious to commit straight-away, so I've disabled a part of that test for the time being (r359867). Nonetheless, could you please take a look at this problem?

I think there is a problem with this patch. I don't believe the TestVLA part would work on any non-darwin platform (it doesn't on linux: http://lab.llvm.org:8014/builders/lldb-x86_64-debian/builds/923), and it works on darwin only accidentally.

The reason the fails on linux is that we don't have a "language runtime" plugin for the "C" language, so there is noone to ask if the variable is a runtime support value, and so we return false from ValueObject::IsRuntimeSupportValue. I believe the reason this works on darwin is because ValueObject::IsRuntimeSupportValue has a fallback (not sure why) which explicitly asks for the ObjC runtime if the desired language is not present. On linux, we don't have an ObjC runtime..

I can think of a couple of solutions here (adding a C language runtime, defaulting to return IsArtificial in ValueObject if no language runtime is present) but none of them seemed too obvious to commit straight-away, so I've disabled a part of that test for the time being (r359867). Nonetheless, could you please take a look at this problem?

Thanks for pointing this out. This should be addressed by r359925.