Skip to content

Commit 02a384a

Browse files
thezhangweimeta-codesync[bot]
authored andcommitted
Exclude Android annotation referenced types from merging
Summary: A mergeable type might be referenced by an Android system annotation like `Ldalvik/annotation/EnclosingMethod;`. Usually `AnnoKill` or `SystemAnnoKill` will wipe out those annotations if not retained for reflection at runtime. If such a referenced type is indeed retained via keep rules, existing mergeability checks will exclude them. However, we might be in a setup where the Android system annotations are simply not removed before merging. In which case, the leftover annotation might still have reference to mergeable types that are actually safe to merge. But the inconsistent state makes it confusing to other optimization passes. Therefore, it's safer, in this case to exclude the annotation referenced types from merging to keep things simple. Types referenced in annotations (except dalvik.annotation.Signature which is handled by TypeStringRewriter) may be retained for reflection purposes at runtime. This change adds a new mergeability check that scans all annotations on classes, fields, methods, and method parameters to find type references that should be excluded from class merging. The check recursively processes encoded values in annotations, handling: - Direct type references (DEVT_TYPE) - Method references (class, return type, parameter types) - Field references (class, field type) - Nested arrays and annotations Reviewed By: ssj933 Differential Revision: D91277070 fbshipit-source-id: 1781c0b1458c39738007d76282ce248ad3dc1661
1 parent 0f1ac09 commit 02a384a

File tree

3 files changed

+161
-0
lines changed

3 files changed

+161
-0
lines changed

service/class-merging/MergeabilityCheck.cpp

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -322,6 +322,145 @@ void MergeabilityChecker::exclude_unsafe_sdk_and_store_refs(
322322
}
323323
}
324324

325+
// Helper to recursively extract all types referenced by an encoded value
326+
// that are in the merging_targets set, and insert them into non_mergeables.
327+
void MergeabilityChecker::collect_referenced_mergeable_types(
328+
const DexEncodedValue* ev,
329+
TypeSet& non_mergeables,
330+
const DexType* anno_type,
331+
const char* context) {
332+
if (ev == nullptr) {
333+
return;
334+
}
335+
336+
auto maybe_insert = [&](const DexType* type) {
337+
if (type != nullptr && m_spec.merging_targets.count(type) > 0) {
338+
auto [it, inserted] = non_mergeables.insert(type);
339+
if (inserted) {
340+
TRACE(CLMG,
341+
5,
342+
"[non mergeable] %s referenced by unhandled annotation %s in %s",
343+
SHOW(type),
344+
SHOW(anno_type),
345+
context);
346+
}
347+
}
348+
};
349+
350+
switch (ev->evtype()) {
351+
case DEVT_TYPE: {
352+
const auto* type_ev = dynamic_cast<const DexEncodedValueType*>(ev);
353+
const auto* type = type::get_element_type_if_array(type_ev->type());
354+
maybe_insert(type);
355+
break;
356+
}
357+
case DEVT_METHOD: {
358+
const auto* method_ev = dynamic_cast<const DexEncodedValueMethod*>(ev);
359+
auto* method = method_ev->method();
360+
if (method != nullptr) {
361+
// Check containing class
362+
maybe_insert(type::get_element_type_if_array(method->get_class()));
363+
// Check return type and parameter types
364+
auto* proto = method->get_proto();
365+
if (proto != nullptr) {
366+
maybe_insert(type::get_element_type_if_array(proto->get_rtype()));
367+
if (auto* args = proto->get_args()) {
368+
for (const auto* arg_type : *args) {
369+
maybe_insert(type::get_element_type_if_array(arg_type));
370+
}
371+
}
372+
}
373+
}
374+
break;
375+
}
376+
case DEVT_FIELD: {
377+
const auto* field_ev = dynamic_cast<const DexEncodedValueField*>(ev);
378+
auto* field = field_ev->field();
379+
if (field != nullptr) {
380+
// Check containing class
381+
maybe_insert(type::get_element_type_if_array(field->get_class()));
382+
// Check field type
383+
maybe_insert(type::get_element_type_if_array(field->get_type()));
384+
}
385+
break;
386+
}
387+
case DEVT_ARRAY: {
388+
const auto* array_ev = dynamic_cast<const DexEncodedValueArray*>(ev);
389+
if (array_ev->evalues() != nullptr) {
390+
for (const auto& elem : *array_ev->evalues()) {
391+
collect_referenced_mergeable_types(elem.get(), non_mergeables,
392+
anno_type, context);
393+
}
394+
}
395+
break;
396+
}
397+
case DEVT_ANNOTATION: {
398+
const auto* anno_ev = dynamic_cast<const DexEncodedValueAnnotation*>(ev);
399+
for (const auto& elem : anno_ev->annotations()) {
400+
collect_referenced_mergeable_types(elem.encoded_value.get(),
401+
non_mergeables, anno_type, context);
402+
}
403+
break;
404+
}
405+
default:
406+
break;
407+
}
408+
}
409+
410+
// We perform the check here mostly for Android system annotations. Since types
411+
// referenced in the Android annotations if kept in the release app are likely
412+
// to be retained for reflection purposes.
413+
void MergeabilityChecker::exclude_unhandled_anno_refs(TypeSet& non_mergeables) {
414+
if (m_spec.merging_targets.empty()) {
415+
return;
416+
}
417+
418+
// dalvik.annotation.Signature is handled by TypeStringRewriter, so we skip
419+
// it. Other annotations that contain type/method/field references to merging
420+
// targets will cause those targets to be excluded from merging.
421+
auto* dalvik_sig = type::dalvik_annotation_Signature();
422+
423+
auto check_anno_set = [&](const DexAnnotationSet* anno_set,
424+
const std::string& context) {
425+
if (anno_set == nullptr) {
426+
return;
427+
}
428+
for (const auto& anno : anno_set->get_annotations()) {
429+
// Skip Signature annotations - TypeStringRewriter handles them.
430+
if (anno->type() == dalvik_sig) {
431+
continue;
432+
}
433+
for (const auto& elem : anno->anno_elems()) {
434+
collect_referenced_mergeable_types(elem.encoded_value.get(),
435+
non_mergeables,
436+
anno->type(),
437+
context.c_str());
438+
}
439+
}
440+
};
441+
442+
for (const auto* cls : m_scope) {
443+
std::string cls_name = show(cls);
444+
check_anno_set(cls->get_anno_set(), cls_name);
445+
446+
for (const auto* field : cls->get_all_fields()) {
447+
check_anno_set(field->get_anno_set(), show(field));
448+
}
449+
450+
for (const auto* method : cls->get_all_methods()) {
451+
std::string method_name = show(method);
452+
check_anno_set(method->get_anno_set(), method_name);
453+
// Also check parameter annotations
454+
const auto* param_annos = method->get_param_anno();
455+
if (param_annos != nullptr) {
456+
for (const auto& param_anno : *param_annos) {
457+
check_anno_set(param_anno.second.get(), method_name);
458+
}
459+
}
460+
}
461+
}
462+
}
463+
325464
TypeSet MergeabilityChecker::get_non_mergeables() {
326465
TypeSet non_mergeables;
327466
size_t prev_size = 0;
@@ -351,6 +490,13 @@ TypeSet MergeabilityChecker::get_non_mergeables() {
351490
non_mergeables.size() - prev_size);
352491
prev_size = non_mergeables.size();
353492

493+
exclude_unhandled_anno_refs(non_mergeables);
494+
TRACE(CLMG,
495+
4,
496+
"Non mergeables (unhandled anno refs) %zu",
497+
non_mergeables.size() - prev_size);
498+
prev_size = non_mergeables.size();
499+
354500
if (m_spec.skip_anonymous_classes) {
355501
for (const auto& type : UnorderedIterable(m_spec.merging_targets)) {
356502
const auto* cls = type_class(type);

service/class-merging/MergeabilityCheck.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,11 @@ class MergeabilityChecker {
4646
void exclude_unsupported_bytecode(TypeSet& non_mergeables);
4747
void exclude_static_fields(TypeSet& non_mergeables);
4848
void exclude_unsafe_sdk_and_store_refs(TypeSet& non_mergeables);
49+
void exclude_unhandled_anno_refs(TypeSet& non_mergeables);
50+
void collect_referenced_mergeable_types(const DexEncodedValue* ev,
51+
TypeSet& non_mergeables,
52+
const DexType* anno_type,
53+
const char* context);
4954

5055
TypeSet exclude_unsupported_bytecode_refs_for(DexMethod* method);
5156
};

test/instr/classmerging-kt-anon-object.config

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,21 @@
11
{
22
"redex" : {
33
"passes" : [
4+
"AnnoKillPass",
45
"ClassMergingPass",
56
"RegAllocPass",
67
"InterDexPass"
78
]
89
},
10+
"AnnoKillPass" : {
11+
"force_kill_annos" : [
12+
"Ldalvik/annotation/EnclosingClass;",
13+
"Ldalvik/annotation/EnclosingMethod;",
14+
"Ldalvik/annotation/InnerClass;",
15+
"Ldalvik/annotation/MemberClasses;",
16+
"Ldalvik/annotation/Throws;"
17+
]
18+
},
919
"ClassMergingPass" : {
1020
"skip_anonymous_classes" : true,
1121
"models" : [

0 commit comments

Comments
 (0)