This is an archive of the discontinued LLVM Phabricator instance.

Differential D38153

Inhibit global lookups for symbols in the IR dynamic checks
ClosedPublic

Authored by spyffe on Sep 21 2017, 1:54 PM.

Details

Reviewers
jingham
clayborg
Commits
rG1b3c43b6dd5b: [Expression Parser] Inhibit global lookups for symbols in the IR dynamic checks
rLLDB314225: [Expression Parser] Inhibit global lookups for symbols in the IR dynamic checks
rL314225: [Expression Parser] Inhibit global lookups for symbols in the IR dynamic checks
Summary

The IR dynamic checks are self-contained functions whose job is to

  • verify that pointers referenced in an expression are valid at runtime; and
  • verify that selectors sent to Objective-C objects by an expression are actually supported by that object.

These dynamic checks forward-declare all the functions they use and should not require any external debug information. The way they ensure this is by marking all the names they use with a dollar sign ($). The expression parser recognizes such symbols and perform no lookups for them.

This patch fixes three issues surrounding the use of the dollar sign:

  • to fix a MIPS issue, the name of the pointer checker was changed from starting with $ to starting with _$, but this was not properly ignored; and
  • the Objective-C object checker used a temporary variable that did not start with $.
  • the Objective-C object checker used an externally-defined struct (struct objc_selector) but didn't need to.

The patch also reformats the string containing the Objective-C object checker, which was mangled horribly when the code was transformed to a uniform width of 80 columns.

Diff Detail

Event Timeline

spyffe created this revision.Sep 21 2017, 1:54 PM
Herald added subscribers: arichardson, aprantl, sdardis. · View Herald TranscriptSep 21 2017, 1:54 PM
clayborg requested changes to this revision.Sep 21 2017, 2:03 PM
clayborg added a subscriber: clayborg.
clayborg added inline comments.
source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
651–652

Should we use a function or macro here? This code is duped below.

source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
812–814

Should we use a function or macro here? This code is duped above

source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
815–831

Since you are modifying this string switch to R"( to make it much clearer:

R"(
extern "C" void *gdb_object_getClass(void *);
extern "C"  int printf(const char *format, ...);
extern "C" void
%s(void *$__lldb_arg_obj, void *$__lldb_arg_selector)
  if ($__lldb_arg_obj == (void *)0)
    return; // nil is ok
  if (!gdb_object_getClass($__lldb_arg_obj)) {
    *((volatile int *)0) = 'ocgc';
  } else if ($__lldb_arg_selector != (void *)0) {
    signed char $responds = (signed char)
        [(id)$__lldb_arg_obj respondsToSelector:
            (void *) $__lldb_arg_selector];
    if ($responds == (signed char) 0)
      *((volatile int *)0) = 'ocgc';
  }
})";
834–852

Use R"( as above.

This revision now requires changes to proceed.Sep 21 2017, 2:03 PM
zturner added a subscriber: zturner.Sep 21 2017, 2:11 PM
zturner added inline comments.
source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
635–652

I might be missing something here, but how about:

std::string name = context.m_decl_name.getAsString();
if (name.empty() || name == "id" || name == "Class")
  return;
if (name.startswith("$") || name.startswith("_$"))
  return;

5 lines instead of ~20.

spyffe marked 5 inline comments as done.Sep 22 2017, 4:20 PM

Thank you. I am testing a new revision with your suggestions.

spyffe updated this revision to Diff 116444.EditedSep 22 2017, 5:18 PM
spyffe edited edge metadata.

Applied Greg's and Zachary's suggestions:

  • Used R"(...)" for multiline strings.
  • Used StringRefs appropriately.
  • Factored out symbol-ignoring into a function shared between ClangASTSource and ClangExpressionDeclMap.
clayborg added a comment.Sep 25 2017, 8:06 AM

Looks good.

spyffe abandoned this revision.Sep 26 2017, 10:30 AM

Committed as LLDB r314225

Closed by commit rL314225: [Expression Parser] Inhibit global lookups for symbols in the IR dynamic checks (authored by spyffe). · Explain WhySep 26 2017, 3:34 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
source/
Plugins/
ExpressionParser/
Clang/
2 lines
3 lines
LanguageRuntime/
ObjC/
AppleObjCRuntime/
112 lines

Diff 116261

source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

Show First 20 Lines • Show All 626 Lines • ▼ Show 20 Linesvoid ClangASTSource::FindExternalVisibleDecls(
NameSearchContext &context, lldb::ModuleSP module_sp, NameSearchContext &context, lldb::ModuleSP module_sp,
CompilerDeclContext &namespace_decl, unsigned int current_id) { CompilerDeclContext &namespace_decl, unsigned int current_id) {
assert(m_ast_context); assert(m_ast_context);
Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS)); Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
SymbolContextList sc_list; SymbolContextList sc_list;
const ConstString name(context.m_decl_name.getAsString().c_str()); const ConstString name(context.m_decl_name.getAsString().c_str());
const char *name_unique_cstr = name.GetCString(); const char *name_unique_cstr = name.GetCString();
static ConstString id_name("id"); static ConstString id_name("id");
static ConstString Class_name("Class"); static ConstString Class_name("Class");
if (name == id_name || name == Class_name) if (name == id_name || name == Class_name)
return; return;
if (name_unique_cstr == NULL) if (name_unique_cstr == NULL)
return; return;
// The ClangASTSource is not responsible for finding $-names. // The ClangASTSource is not responsible for finding $-names.
if (name_unique_cstr[0] == '$') if (name_unique_cstr[0] == '$')
return; return;
if (name_unique_cstr[0] == '_' && name_unique_cstr[1] == '$')
return;
clayborgUnsubmitted
Done
ReplyInline Actions

Should we use a function or macro here? This code is duped below.

clayborg: Should we use a function or macro here? This code is duped below.
zturnerUnsubmitted
Done
ReplyInline Actions

I might be missing something here, but how about:

std::string name = context.m_decl_name.getAsString();
if (name.empty() || name == "id" || name == "Class")
  return;
if (name.startswith("$") || name.startswith("_$"))
  return;

5 lines instead of ~20.

zturner: I might be missing something here, but how about: ``` std::string name = context.m_decl_name.
if (module_sp && namespace_decl) { if (module_sp && namespace_decl) {
CompilerDeclContext found_namespace_decl; CompilerDeclContext found_namespace_decl;
SymbolVendor *symbol_vendor = module_sp->GetSymbolVendor(); SymbolVendor *symbol_vendor = module_sp->GetSymbolVendor();
if (symbol_vendor) { if (symbol_vendor) {
SymbolContext null_sc; SymbolContext null_sc;
▲ Show 20 Lines • Show All 992 Lines • Show Last 20 Lines

source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

Show First 20 Lines • Show All 803 Lines • ▼ Show 20 Lines do {
if (log) if (log)
log->Printf(" CEDM::FEVD[%u] Found persistent decl %s", current_id, log->Printf(" CEDM::FEVD[%u] Found persistent decl %s", current_id,
name.GetCString()); name.GetCString());
context.AddNamedDecl(parser_named_decl); context.AddNamedDecl(parser_named_decl);
} while (0); } while (0);
} }
if (name_unique_cstr[0] == '_' && name_unique_cstr[1] == '$')
return;
clayborgUnsubmitted
Done
ReplyInline Actions

Should we use a function or macro here? This code is duped above

clayborg: Should we use a function or macro here? This code is duped above
if (name_unique_cstr[0] == '$' && !namespace_decl) { if (name_unique_cstr[0] == '$' && !namespace_decl) {
static ConstString g_lldb_class_name("$__lldb_class"); static ConstString g_lldb_class_name("$__lldb_class");
if (name == g_lldb_class_name) { if (name == g_lldb_class_name) {
// Clang is looking for the type of "this" // Clang is looking for the type of "this"
if (frame == NULL) if (frame == NULL)
return; return;
▲ Show 20 Lines • Show All 992 Lines • Show Last 20 Lines

source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp

Show First 20 Lines • Show All 806 Lines • ▼ Show 20 Lines
} }
UtilityFunction *AppleObjCRuntimeV2::CreateObjectChecker(const char *name) { UtilityFunction *AppleObjCRuntimeV2::CreateObjectChecker(const char *name) {
char check_function_code[2048]; char check_function_code[2048];
int len = 0; int len = 0;
if (m_has_object_getClass) { if (m_has_object_getClass) {
len = ::snprintf(check_function_code, sizeof(check_function_code), len = ::snprintf(check_function_code, sizeof(check_function_code),
"extern \"C\" void *gdb_object_getClass(void *); " "extern \"C\" void *gdb_object_getClass(void *); \n"
" \n" "extern \"C\" int printf(const char *format, ...); \n"
"extern \"C\" int printf(const char *format, ...); " "extern \"C\" void \n"
" \n" "%s(void *$__lldb_arg_obj, void *$__lldb_arg_selector) {\n"
"extern \"C\" void " " if ($__lldb_arg_obj == (void *)0) \n"
" \n" " return; // nil is ok \n"
"%s(void *$__lldb_arg_obj, void *$__lldb_arg_selector) " " if (!gdb_object_getClass($__lldb_arg_obj)) { \n"
" \n" " *((volatile int *)0) = 'ocgc'; \n"
"{ " " } else if ($__lldb_arg_selector != (void *)0) { \n"
" \n" " signed char $responds = (signed char) \n"
" if ($__lldb_arg_obj == (void *)0) " " [(id)$__lldb_arg_obj respondsToSelector: \n"
" \n" " (void *) $__lldb_arg_selector]; \n"
" return; // nil is ok " " if ($responds == (signed char) 0) \n"
" \n" " *((volatile int *)0) = 'ocgc'; \n"
" if (!gdb_object_getClass($__lldb_arg_obj)) " " } \n"
" \n" "} \n"
" *((volatile int *)0) = 'ocgc'; " , name);
clayborgUnsubmitted
Done
ReplyInline Actions

Since you are modifying this string switch to R"( to make it much clearer:

R"(
extern "C" void *gdb_object_getClass(void *);
extern "C"  int printf(const char *format, ...);
extern "C" void
%s(void *$__lldb_arg_obj, void *$__lldb_arg_selector)
  if ($__lldb_arg_obj == (void *)0)
    return; // nil is ok
  if (!gdb_object_getClass($__lldb_arg_obj)) {
    *((volatile int *)0) = 'ocgc';
  } else if ($__lldb_arg_selector != (void *)0) {
    signed char $responds = (signed char)
        [(id)$__lldb_arg_obj respondsToSelector:
            (void *) $__lldb_arg_selector];
    if ($responds == (signed char) 0)
      *((volatile int *)0) = 'ocgc';
  }
})";
clayborg: Since you are modifying this string switch to R"( to make it much clearer: ``` R"( extern "C"…
" \n"
" else if ($__lldb_arg_selector != (void *)0) "
" \n"
" { "
" \n"
" signed char responds = (signed char) [(id) "
"$__lldb_arg_obj \n"
" "
"respondsToSelector: \n"
" (struct "
"objc_selector *) $__lldb_arg_selector]; \n"
" if (responds == (signed char) 0) "
" \n"
" *((volatile int *)0) = 'ocgc'; "
" \n"
" } "
" \n"
"} "
" \n",
name);
} else { } else {
len = ::snprintf(check_function_code, sizeof(check_function_code), len = ::snprintf(check_function_code, sizeof(check_function_code),
"extern \"C\" void *gdb_class_getClass(void *); " "extern \"C\" void *gdb_class_getClass(void *); \n"
" \n" "extern \"C\" int printf(const char *format, ...); \n"
"extern \"C\" int printf(const char *format, ...); " "extern \"C\" void \n"
" \n" "%s(void *$__lldb_arg_obj, void *$__lldb_arg_selector) {\n"
"extern \"C\" void " " if ($__lldb_arg_obj == (void *)0) \n"
" \n" " return; // nil is ok \n"
"%s(void *$__lldb_arg_obj, void *$__lldb_arg_selector) " " void **$isa_ptr = (void **)$__lldb_arg_obj; \n"
" \n" " if (*$isa_ptr == (void *)0 || \n"
"{ "
" \n"
" if ($__lldb_arg_obj == (void *)0) "
" \n"
" return; // nil is ok "
" \n"
" void **$isa_ptr = (void **)$__lldb_arg_obj; "
" \n"
" if (*$isa_ptr == (void *)0 || "
"!gdb_class_getClass(*$isa_ptr)) \n" " !gdb_class_getClass(*$isa_ptr)) \n"
" *((volatile int *)0) = 'ocgc'; " " *((volatile int *)0) = 'ocgc'; \n"
" \n" " else if ($__lldb_arg_selector != (void *)0) { \n"
" else if ($__lldb_arg_selector != (void *)0) " " signed char $responds = (signed char) \n"
" \n" " [(id)$__lldb_arg_obj respondsToSelector: \n"
" { " " (void *) $__lldb_arg_selector]; \n"
" \n" " if ($responds == (signed char) 0) \n"
" signed char responds = (signed char) [(id) " " *((volatile int *)0) = 'ocgc'; \n"
"$__lldb_arg_obj \n" " } \n"
" " "} \n"
"respondsToSelector: \n" , name);
clayborgUnsubmitted
Done
ReplyInline Actions

Use R"( as above.

clayborg: Use R"( as above.
" (struct "
"objc_selector *) $__lldb_arg_selector]; \n"
" if (responds == (signed char) 0) "
" \n"
" *((volatile int *)0) = 'ocgc'; "
" \n"
" } "
" \n"
"} "
" \n",
name);
} }
assert(len < (int)sizeof(check_function_code)); assert(len < (int)sizeof(check_function_code));
UNUSED_IF_ASSERT_DISABLED(len); UNUSED_IF_ASSERT_DISABLED(len);
Status error; Status error;
return GetTargetRef().GetUtilityFunctionForLanguage( return GetTargetRef().GetUtilityFunctionForLanguage(
check_function_code, eLanguageTypeObjC, name, error); check_function_code, eLanguageTypeObjC, name, error);
▲ Show 20 Lines • Show All 992 Lines • Show Last 20 Lines