This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Upstream support for Foundation constant classes
ClosedPublic

Authored by JDevlieghere on Aug 6 2021, 11:40 AM.

Details

Summary

Upstream support for NSConstantArray, NSConstantIntegerNumber, NSConstant{Float,Double}Number and NSConstantDictionary. We would've upstreamed this earlier but testing it requires -fno-constant-nsnumber-literals, -fno-constant-nsarray-literals and -fno-constant-nsdictionary-literals which haven't been upstreamed yet. As a temporary workaround use the system compiler (xcrun clang) for the constant variant of the tests.

I'm just upstreaming this. The patch and the tests were all authored by Fred Riss.

Diff Detail

Event Timeline

JDevlieghere requested review of this revision.Aug 6 2021, 11:40 AM
JDevlieghere created this revision.
jingham requested changes to this revision.Aug 6 2021, 12:14 PM

Just a few little nits.

lldb/source/Plugins/Language/ObjC/Cocoa.cpp
509

Shouldn't this be done with a DataExtractor? The code is assuming that you can get a valid host float by pasting target bytes into a host float. While most likely everybody is using the same float & double representations these days, that doesn't seem like something we should rely on.

lldb/source/Plugins/Language/ObjC/NSDictionary.cpp
908–913

These are pointers, shouldn't we be using ReadPointerFromMemory?

This revision now requires changes to proceed.Aug 6 2021, 12:14 PM
JDevlieghere marked 2 inline comments as done.
  • Use ReadPointerFromMemory
lldb/source/Plugins/Language/ObjC/Cocoa.cpp
509

This seems pretty common across the existing code. I agree we should fix that, but probably as a follow-up patch that fixes this across the whole file.

jingham accepted this revision.Aug 6 2021, 12:58 PM

If the float/double pattern wasn't introduced here, then it's fine by me to fix it in a follow-on patch.
Otherwise, LGTM.

This revision is now accepted and ready to land.Aug 6 2021, 12:58 PM
This revision was landed with ongoing or failed builds.Aug 6 2021, 4:09 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2021, 4:09 PM

This change breaks various build bots with a missing file. See below. Reverting shortly.

For example:

https://lab.llvm.org/buildbot/#/builders/68/builds/16816

/usr/bin/clang++ -DGTEST_HAS_RTTI=0 -DHAVE_ROUND -D_DEBUG -D_GNU_SOURCE -DSTDC_CONSTANT_MACROS -DSTDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/lldb/source/Plugins/Language/ObjC -I/home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/source/Plugins/Language/ObjC -Itools/lldb/source -I/home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/include -Itools/lldb/include -Iinclude -I/home/worker/2.0.1/lldb-x86_64-debian/llvm-project/llvm/include -I/usr/include/python3.7m -I/home/worker/2.0.1/lldb-x86_64-debian/llvm-project/llvm/../clang/include -Itools/lldb/../clang/include -I/home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/source/. -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -Wno-deprecated-declarations -Wno-unknown-pragmas -Wno-strict-aliasing -Wno-deprecated-register -Wno-vla-extension -O3 -DNDEBUG -fno-exceptions -fno-rtti -UNDEBUG -std=c++14 -MD -MT tools/lldb/source/Plugins/Language/ObjC/CMakeFiles/lldbPluginObjCLanguage.dir/Cocoa.cpp.o -MF tools/lldb/source/Plugins/Language/ObjC/CMakeFiles/lldbPluginObjCLanguage.dir/Cocoa.cpp.o.d -o tools/lldb/source/Plugins/Language/ObjC/CMakeFiles/lldbPluginObjCLanguage.dir/Cocoa.cpp.o -c /home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/source/Plugins/Language/ObjC/Cocoa.cpp
/home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/source/Plugins/Language/ObjC/Cocoa.cpp:10:10: fatal error: 'objc/runtime.h' file not found
#include "objc/runtime.h"

^~~~~~~~~~~~~~~~

Yep, I'm looking into it.

Thanks for looking. Reverted with 4e5af6ef48590e7248e344ddabf245bb3de71c51.

What's the point of giving a heads up if you're goin to revert it immediately, even after you know I'm already looking into it? To be clear, I have no issue with the revert-to-green policy, I do it all the time myself, but if you're going to do that, you might as well revert it right away and skip the heads up.

Some people complain if I revert without notification. So I always notify as soon as I find the patch with the problem. Some reverts are more complicated than others too.

I guess it adds more noise but I try to default to more communication over less.

Some people complain if I revert without notification. So I always notify as soon as I find the patch with the problem. Some reverts are more complicated than others too.

I guess it adds more noise but I try to default to more communication over less.

Gotcha, I was genuinely curious, thanks for the context.