From 2e164dc8db462025e2f33fcef24f7b537398d690 Mon Sep 17 00:00:00 2001 From: Max Trivedi Date: Fri, 24 Jan 2025 22:43:50 -0800 Subject: [PATCH] add #ICInaccessibleSpecialCase memoize option Summary: Some DEIC special case memoize functions require access to the implicit context while not emitting implicit context key (accesses IC, not sharded by IC). Adding the memoize option to do that. Reviewed By: jano Differential Revision: D68542539 fbshipit-source-id: b501b5c4332b843a72e9f6517b83e89a9ecaca52 --- .../hackc/emitter/emit_memoize_function.rs | 7 +- .../src/hackc/emitter/emit_memoize_method.rs | 18 +++- hphp/hack/src/hackc/hhbc/attribute.rs | 4 +- hphp/hack/src/naming/naming_special_names.rs | 9 +- hphp/hack/src/parser/lowerer/lowerer.rs | 13 ++- hphp/hack/src/parser/syntax_error.rs | 11 ++- hphp/runtime/ext/hh/ext_implicit_context.php | 1 + .../runtime/ext/reflection/ext_reflection.cpp | 6 +- hphp/runtime/vm/func-emitter.cpp | 3 + hphp/runtime/vm/func-inl.h | 4 - hphp/runtime/vm/func.h | 3 +- ...ic-inaccessible-special-case-coeffects.php | 12 +++ ...essible-special-case-coeffects.php.expectf | 2 + .../ic-inaccessible-special-case.php | 92 +++++++++++++++++++ .../ic-inaccessible-special-case.php.expect | 6 ++ 15 files changed, 168 insertions(+), 23 deletions(-) create mode 100644 hphp/test/slow/implicit-context/ic-inaccessible-special-case-coeffects.php create mode 100644 hphp/test/slow/implicit-context/ic-inaccessible-special-case-coeffects.php.expectf create mode 100644 hphp/test/slow/implicit-context/ic-inaccessible-special-case.php create mode 100644 hphp/test/slow/implicit-context/ic-inaccessible-special-case.php.expect diff --git a/hphp/hack/src/hackc/emitter/emit_memoize_function.rs b/hphp/hack/src/hackc/emitter/emit_memoize_function.rs index d5fec30af1c5d7..83595d8e7aeea5 100644 --- a/hphp/hack/src/hackc/emitter/emit_memoize_function.rs +++ b/hphp/hack/src/hackc/emitter/emit_memoize_function.rs @@ -93,11 +93,14 @@ pub(crate) fn emit_wrapper_function<'a, 'd>( .any(|tp| tp.reified.is_reified() || tp.reified.is_soft_reified()); let coeffects = Coeffects::from_ast(f.ctxs.as_ref(), &f.params, &fd.tparams, vec![]); let should_emit_implicit_context = hhbc::is_keyed_by_ic_memoize(attributes.iter()); + // #ICInaccessibleSpecialCase won't shard by IC but will access IC + let is_inaccessible_special_case = hhbc::is_inaccessible_special_case(attributes.iter()); // This fn either has IC unoptimizable static coeffects, or has any dynamic coeffects let has_ic_unoptimizable_coeffects: bool = coeffects.has_ic_unoptimizable_coeffects(); - let should_make_ic_inaccessible: bool = - !should_emit_implicit_context && has_ic_unoptimizable_coeffects; + let should_make_ic_inaccessible: bool = !is_inaccessible_special_case + && !should_emit_implicit_context + && has_ic_unoptimizable_coeffects; let mut env = Env::default(Arc::clone(&fd.namespace)).with_scope(scope); let (instrs, decl_vars) = make_memoize_function_code( diff --git a/hphp/hack/src/hackc/emitter/emit_memoize_method.rs b/hphp/hack/src/hackc/emitter/emit_memoize_method.rs index fe7d3742ce679a..8d273fbf8814dd 100644 --- a/hphp/hack/src/hackc/emitter/emit_memoize_method.rs +++ b/hphp/hack/src/hackc/emitter/emit_memoize_method.rs @@ -161,6 +161,10 @@ fn make_memoize_wrapper_method<'a, 'd>( Flags::SHOULD_EMIT_IMPLICIT_CONTEXT, hhbc::is_keyed_by_ic_memoize(attributes.iter()), ); + arg_flags.set( + Flags::IS_IC_INACCESSIBLE_SPECIAL_CASE, + hhbc::is_inaccessible_special_case(attributes.iter()), + ); let mut args = Args { info, method, @@ -302,6 +306,8 @@ fn make_memoize_method_with_params_code<'a, 'd>( // so the first unnamed local is parameter count + 1 when there are reified generics. let is_reified = args.flags.contains(Flags::IS_REIFIED); let add_reified = usize::from(is_reified); + // #ICInaccessibleSpecialCase won't shard by IC but will access IC + let is_inaccessible_special_case = args.flags.contains(Flags::IS_IC_INACCESSIBLE_SPECIAL_CASE); let should_emit_implicit_context = args.flags.contains(Flags::SHOULD_EMIT_IMPLICIT_CONTEXT); let add_implicit_context = usize::from(should_emit_implicit_context); let first_unnamed_idx = param_count + add_reified; @@ -368,8 +374,9 @@ fn make_memoize_method_with_params_code<'a, 'd>( let ic_stash_local = Local::new((key_count) as usize + first_unnamed_idx); // This fn either has IC unoptimizable static coeffects, or has any dynamic coeffects let has_ic_unoptimizable_coeffects: bool = coeffects.has_ic_unoptimizable_coeffects(); - let should_make_ic_inaccessible: bool = - !should_emit_implicit_context && has_ic_unoptimizable_coeffects; + let should_make_ic_inaccessible: bool = !is_inaccessible_special_case + && !should_emit_implicit_context + && has_ic_unoptimizable_coeffects; let instrs = InstrSeq::gather(vec![ begin_label, emit_body::emit_method_prolog(emitter, env, pos, hhas_params, args.params, &[])?, @@ -459,9 +466,12 @@ fn make_memoize_method_no_params_code<'a, 'd>( }, None, ); + // #ICInaccessibleSpecialCase won't shard by IC but will access IC + let is_inaccessible_special_case = args.flags.contains(Flags::IS_IC_INACCESSIBLE_SPECIAL_CASE); let ic_stash_local = Local::new(0); // we are in a no parameter function that sets no zoned IC either, default to what coeffects suggest - let should_make_ic_inaccessible: bool = coeffects.has_ic_unoptimizable_coeffects(); + let should_make_ic_inaccessible: bool = + coeffects.has_ic_unoptimizable_coeffects() && !is_inaccessible_special_case; let instrs = InstrSeq::gather(vec![ deprecation_body, if args.method.static_ { @@ -583,6 +593,6 @@ bitflags! { const IS_ASYNC = 1 << 4; const SHOULD_EMIT_IMPLICIT_CONTEXT = 1 << 5; const SHOULD_MAKE_IC_INACCESSIBLE = 1 << 6; - const SHOULD_SOFT_MAKE_IC_INACCESSIBLE = 1 << 7; + const IS_IC_INACCESSIBLE_SPECIAL_CASE = 1 << 7; } } diff --git a/hphp/hack/src/hackc/hhbc/attribute.rs b/hphp/hack/src/hackc/hhbc/attribute.rs index 155358a8862a13..422919cab362e2 100644 --- a/hphp/hack/src/hackc/hhbc/attribute.rs +++ b/hphp/hack/src/hackc/hhbc/attribute.rs @@ -86,8 +86,8 @@ pub fn is_keyed_by_ic_memoize(attrs: impl AsRef<[Attribute]>) -> bool { is_memoize_with(attrs, "KeyedByIC") } -pub fn is_make_ic_inaccessible_memoize(attrs: impl AsRef<[Attribute]>) -> bool { - is_memoize_with(attrs, "MakeICInaccessible") +pub fn is_inaccessible_special_case(attrs: impl AsRef<[Attribute]>) -> bool { + is_memoize_with(attrs, "ICInaccessibleSpecialCase") } fn is_foldable(attr: &Attribute) -> bool { diff --git a/hphp/hack/src/naming/naming_special_names.rs b/hphp/hack/src/naming/naming_special_names.rs index db6825f0bc9c7a..2c275b4fdbec70 100644 --- a/hphp/hack/src/naming/naming_special_names.rs +++ b/hphp/hack/src/naming/naming_special_names.rs @@ -8,7 +8,6 @@ */ /** Module consisting of the special names known to the typechecker */ - pub mod classes { pub const PARENT: &str = "parent"; @@ -387,7 +386,13 @@ pub mod memoize_option { pub const MAKE_IC_INACCESSSIBLE: &str = "MakeICInaccessible"; - pub static _ALL: &[&str] = &[KEYED_BY_IC, MAKE_IC_INACCESSSIBLE]; + pub const IC_INACCESSSIBLE_SPECIAL_CASE: &str = "ICInaccessibleSpecialCase"; + + pub static _ALL: &[&str] = &[ + KEYED_BY_IC, + MAKE_IC_INACCESSSIBLE, + IC_INACCESSSIBLE_SPECIAL_CASE, + ]; lazy_static! { static ref VALID_SET: HashSet<&'static str> = _ALL.iter().copied().collect(); diff --git a/hphp/hack/src/parser/lowerer/lowerer.rs b/hphp/hack/src/parser/lowerer/lowerer.rs index 7c01217602450d..8b86a687440b3e 100644 --- a/hphp/hack/src/parser/lowerer/lowerer.rs +++ b/hphp/hack/src/parser/lowerer/lowerer.rs @@ -5654,9 +5654,12 @@ fn check_effect_memoized<'a>( ) } } - // #(Soft)?MakeICInaccessible can only be used on functions with defaults if let Some(u) = user_attributes.iter().find(|u| { is_memoize_attribute_with_flavor(u, Some(sn::memoize_option::MAKE_IC_INACCESSSIBLE)) + || is_memoize_attribute_with_flavor( + u, + Some(sn::memoize_option::IC_INACCESSSIBLE_SPECIAL_CASE), + ) }) { if !has_any_context( contexts, @@ -5669,7 +5672,13 @@ fn check_effect_memoized<'a>( raise_parsing_error_pos( &u.name.0, env, - &syntax_error::memoize_make_ic_inaccessible_without_defaults(kind), + &syntax_error::memoize_make_ic_inaccessible_without_defaults( + kind, + is_memoize_attribute_with_flavor( + u, + Some(sn::memoize_option::IC_INACCESSSIBLE_SPECIAL_CASE), + ), + ), ) } } diff --git a/hphp/hack/src/parser/syntax_error.rs b/hphp/hack/src/parser/syntax_error.rs index ddd05985a3d961..800e4d0a42a742 100644 --- a/hphp/hack/src/parser/syntax_error.rs +++ b/hphp/hack/src/parser/syntax_error.rs @@ -1068,10 +1068,15 @@ pub fn policy_sharded_memoized_without_policied(kind: &str) -> Error { )) } -pub fn memoize_make_ic_inaccessible_without_defaults(kind: &str) -> Error { +pub fn memoize_make_ic_inaccessible_without_defaults(kind: &str, is_special_case: bool) -> Error { + let context = if is_special_case { + "#ICInaccessibleSpecialCase" + } else { + "#MakeICInaccessible" + }; Cow::Owned(format!( - "This {} requires the defaults, leak_safe_shallow, or leak_safe_local context to be memoized using #MakeICInaccessible", - kind + "This {} requires the defaults, leak_safe_shallow, or leak_safe_local context to be memoized using {}", + kind, context )) } diff --git a/hphp/runtime/ext/hh/ext_implicit_context.php b/hphp/runtime/ext/hh/ext_implicit_context.php index 21d74d2147c31c..66c1893ec9779d 100644 --- a/hphp/runtime/ext/hh/ext_implicit_context.php +++ b/hphp/runtime/ext/hh/ext_implicit_context.php @@ -175,6 +175,7 @@ protected static function get()[this::CRun]: ?this::T { enum class MemoizeOption: string { string KeyedByIC = 'KeyedByIC'; string MakeICInaccessible = 'MakeICInaccessible'; + string ICInaccessibleSpecialCase = 'ICInaccessibleSpecialCase'; } } // namespace HH diff --git a/hphp/runtime/ext/reflection/ext_reflection.cpp b/hphp/runtime/ext/reflection/ext_reflection.cpp index f90768b7d7b062..1f9e7fb8c382db 100644 --- a/hphp/runtime/ext/reflection/ext_reflection.cpp +++ b/hphp/runtime/ext/reflection/ext_reflection.cpp @@ -939,7 +939,8 @@ const StaticString s_MemoizeLSB("__MemoizeLSB"), s_systemlib_create_opaque_value("__SystemLib\\create_opaque_value"), s_KeyedByIC("KeyedByIC"), - s_MakeICInaccessible("MakeICInaccessible"); + s_MakeICInaccessible("MakeICInaccessible"), + s_ICInaccessibleSpecialCase("ICInaccessibleSpecialCase"); ALWAYS_INLINE static Array get_function_user_attributes(const Func* func) { @@ -961,7 +962,8 @@ static Array get_function_user_attributes(const Func* func) { } else { auto const sd = tv.m_data.pstr; if (sd->same(s_KeyedByIC.get()) || - sd->same(s_MakeICInaccessible.get())) { + sd->same(s_MakeICInaccessible.get()) || + sd->same(s_ICInaccessibleSpecialCase.get())) { if (Cfg::Eval::EmitNativeEnumClassLabels) { args.append(make_tv(sd)); } else { diff --git a/hphp/runtime/vm/func-emitter.cpp b/hphp/runtime/vm/func-emitter.cpp index 523cd2ab4714e6..3ca2da30d60a16 100644 --- a/hphp/runtime/vm/func-emitter.cpp +++ b/hphp/runtime/vm/func-emitter.cpp @@ -209,6 +209,7 @@ const StaticString s_MemoizeLSB("__MemoizeLSB"), s_KeyedByIC("KeyedByIC"), s_MakeICInaccessible("MakeICInaccessible"), + s_ICInaccessibleSpecialCase("ICInaccessibleSpecialCase"), s_SoftInternal("__SoftInternal"); namespace { @@ -303,6 +304,8 @@ Func* FuncEmitter::create(Unit& unit, PreClass* preClass /* = NULL */) const { icType = Func::MemoizeICType::KeyedByIC; } else if (elem.m_data.pstr->same(s_MakeICInaccessible.get())) { icType = Func::MemoizeICType::MakeICInaccessible; + } else if (elem.m_data.pstr->same(s_ICInaccessibleSpecialCase.get())) { + icType = Func::MemoizeICType::ICInaccessibleSpecialCase; } else { assertx(false && "invalid string"); } diff --git a/hphp/runtime/vm/func-inl.h b/hphp/runtime/vm/func-inl.h index 0a30470a209f08..176640e8f0739b 100644 --- a/hphp/runtime/vm/func-inl.h +++ b/hphp/runtime/vm/func-inl.h @@ -577,10 +577,6 @@ inline bool Func::isKeyedByImplicitContextMemoize() const { return memoizeICType() == MemoizeICType::KeyedByIC; } -inline bool Func::isMakeICInaccessibleMemoize() const { - return memoizeICType() == MemoizeICType::MakeICInaccessible; -} - inline bool Func::isMemoizeImpl() const { return isMemoizeImplName(name()); diff --git a/hphp/runtime/vm/func.h b/hphp/runtime/vm/func.h index e264284287337f..552cb06eae06f5 100644 --- a/hphp/runtime/vm/func.h +++ b/hphp/runtime/vm/func.h @@ -721,6 +721,7 @@ struct Func final { NoIC = 0, KeyedByIC = 1, MakeICInaccessible = 2, + ICInaccessibleSpecialCase = 3, }; MemoizeICType memoizeICType() const; @@ -729,8 +730,6 @@ struct Func final { bool isKeyedByImplicitContextMemoize() const; - bool isMakeICInaccessibleMemoize() const; - /* * Is this string the name of a memoize implementation. */ diff --git a/hphp/test/slow/implicit-context/ic-inaccessible-special-case-coeffects.php b/hphp/test/slow/implicit-context/ic-inaccessible-special-case-coeffects.php new file mode 100644 index 00000000000000..529e9e36b12bf3 --- /dev/null +++ b/hphp/test/slow/implicit-context/ic-inaccessible-special-case-coeffects.php @@ -0,0 +1,12 @@ +> +function memo_inaccessible_sc_leaksafe($a, $b)[leak_safe]: mixed{ + echo "memo_inaccessible_sc_leaksafe: $a, $b \n"; +} + +<<__EntryPoint>> +function main() { + memo_inaccessible_sc_leaksafe(1, 2); +} diff --git a/hphp/test/slow/implicit-context/ic-inaccessible-special-case-coeffects.php.expectf b/hphp/test/slow/implicit-context/ic-inaccessible-special-case-coeffects.php.expectf new file mode 100644 index 00000000000000..3bd223399e84da --- /dev/null +++ b/hphp/test/slow/implicit-context/ic-inaccessible-special-case-coeffects.php.expectf @@ -0,0 +1,2 @@ + +Fatal error: This function requires the defaults, leak_safe_shallow, or leak_safe_local context to be memoized using #ICInaccessibleSpecialCase in %s/hphp/test/slow/implicit-context/ic-inaccessible-special-case-coeffects.php on line 4 diff --git a/hphp/test/slow/implicit-context/ic-inaccessible-special-case.php b/hphp/test/slow/implicit-context/ic-inaccessible-special-case.php new file mode 100644 index 00000000000000..9658860f0a88ec --- /dev/null +++ b/hphp/test/slow/implicit-context/ic-inaccessible-special-case.php @@ -0,0 +1,92 @@ +name(); + } + public function name()[]: string { return static::class; } +} + +abstract final class ClassContext extends HH\ImplicitContext { + const type T = Base; + const bool IS_MEMO_SENSITIVE = true; + const ctx CRun = [defaults]; + public static function start(Base $context, (function (): int) $f)[this::CRun, ctx $f] { + return parent::runWith($context, $f); + } + public static function getContext()[this::CRun]: Base { + return parent::get() as nonnull; + } + public static function exists()[this::CRun]: bool { + return parent::exists() as bool; + } +} + +class A extends Base {} + +class B extends Base { + <<__Memoize(#KeyedByIC)>> + public function memo_kbic($a, $b)[defaults]: mixed { + $context = ClassContext::getContext()->name(); + echo "args: $a, $b name: $context\n"; + } + + <<__Memoize(#MakeICInaccessible)>> + public function memo_inaccessible($a, $b)[defaults]: mixed { + $context = ClassContext::getContext()->name(); + echo "args: $a, $b name: $context\n"; + } + + <<__Memoize(#ICInaccessibleSpecialCase)>> + public function memo_inaccessible_sc($a, $b)[defaults]: mixed { + $context = ClassContext::getContext()->name(); + echo "args: $a, $b name: $context\n"; + } + +} + + +<<__Memoize(#KeyedByIC)>> +function memo_kbic($a, $b)[defaults]: mixed{ + $context = ClassContext::getContext()->name(); + echo "args: $a, $b name: $context\n"; +} + +<<__Memoize(#MakeICInaccessible)>> +function memo_inaccessible($a, $b)[defaults]: mixed{ + $context = ClassContext::getContext()->name(); + echo "args: $a, $b name: $context\n"; +} + +<<__Memoize(#ICInaccessibleSpecialCase)>> +function memo_inaccessible_sc($a, $b)[defaults]: mixed{ + $context = ClassContext::getContext()->name(); + echo "args: $a, $b name: $context\n"; +} + +function f()[defaults]: mixed{ + $klass_b = new B; + $tryout = function($memo_function, $a, $b) use ($klass_b) { + try { + $memo_function($a, $b); + } catch (Exception $e) { + echo "Function $memo_function throws: ".$e->getMessage() . "\n"; + } + + try { + $klass_b->$memo_function($a, $b); + } catch (Exception $e) { + echo "Method B->$memo_function throws: ".$e->getMessage() . "\n"; + + } + }; + $tryout('memo_kbic', 1, 2); + $tryout('memo_inaccessible', 3, 4); + $tryout('memo_inaccessible_sc', 5, 6); +} + + +<<__EntryPoint>> +function main(): mixed{ + ClassContext::start(new A, f<>); +} diff --git a/hphp/test/slow/implicit-context/ic-inaccessible-special-case.php.expect b/hphp/test/slow/implicit-context/ic-inaccessible-special-case.php.expect new file mode 100644 index 00000000000000..a5e700a45eb0b3 --- /dev/null +++ b/hphp/test/slow/implicit-context/ic-inaccessible-special-case.php.expect @@ -0,0 +1,6 @@ +args: 1, 2 name: A +args: 1, 2 name: A +Function memo_inaccessible throws: Implicit context is set to inaccessible +Method B->memo_inaccessible throws: Implicit context is set to inaccessible +args: 5, 6 name: A +args: 5, 6 name: A