Skip to content

src: start annotating native code side effect #21458

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
TimothyGu wants to merge 5 commits into from
Closed

src: start annotating native code side effect #21458

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7132853
src: start annotating native code side effect
TimothyGu Jun 22, 2018
c5f4cb5
fixup! src: start annotating native code side effect
TimothyGu Jun 22, 2018
d418bb8
fixup! fixup! src: start annotating native code side effect
TimothyGu Jun 22, 2018
a129a6f
fixup! src: start annotating native code side effect
TimothyGu Jun 23, 2018
c43658d
fixup! fixup! src: start annotating native code side effect
TimothyGu Jun 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/cares_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ using v8::Integer;
using v8::Local;
using v8::Null;
using v8::Object;
using v8::SideEffectType;
using v8::String;
using v8::Value;

Expand Down Expand Up @@ -2107,8 +2108,10 @@ void Initialize(Local<Object> target,

env->SetMethod(target, "getaddrinfo", GetAddrInfo);
env->SetMethod(target, "getnameinfo", GetNameInfo);
env->SetMethod(target, "isIPv6", IsIPv6);
env->SetMethod(target, "canonicalizeIP", CanonicalizeIP);
env->SetMethod(target, "isIPv6", IsIPv6,
SideEffectType::kHasNoSideEffect);
env->SetMethod(target, "canonicalizeIP", CanonicalizeIP,
SideEffectType::kHasNoSideEffect);

env->SetMethod(target, "strerror", StrError);

Expand Down Expand Up @@ -2165,7 +2168,8 @@ void Initialize(Local<Object> target,
env->SetProtoMethod(channel_wrap, "querySoa", Query<QuerySoaWrap>);
env->SetProtoMethod(channel_wrap, "getHostByAddr", Query<GetHostByAddrWrap>);

env->SetProtoMethod(channel_wrap, "getServers", GetServers);
env->SetProtoMethod(channel_wrap, "getServers", GetServers,
SideEffectType::kHasNoSideEffect);
env->SetProtoMethod(channel_wrap, "setServers", SetServers);
env->SetProtoMethod(channel_wrap, "cancel", Cancel);

Expand Down
33 changes: 23 additions & 10 deletions src/env-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -675,17 +675,24 @@ inline void Environment::ThrowUVException(int errorno,
inline v8::Local<v8::FunctionTemplate>
Environment::NewFunctionTemplate(v8::FunctionCallback callback,
v8::Local<v8::Signature> signature,
v8::ConstructorBehavior behavior) {
v8::ConstructorBehavior behavior,
v8::SideEffectType side_effect_type) {
v8::Local<v8::External> external = as_external();
return v8::FunctionTemplate::New(isolate(), callback, external,
signature, 0, behavior);
signature, 0, behavior, side_effect_type);
}

inline void Environment::SetMethod(v8::Local<v8::Object> that,
const char* name,
v8::FunctionCallback callback) {
v8::FunctionCallback callback,
v8::SideEffectType side_effect_type) {
v8::Local<v8::Function> function =
NewFunctionTemplate(callback)->GetFunction();
NewFunctionTemplate(callback,
v8::Local<v8::Signature>(),
// TODO(TimothyGu): Investigate if SetMethod is ever
// used for constructors.
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Jun 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's easy to check, isn't it? Or do you suspect make test will miss instances?

Copy link
Copy Markdown
Member Author

@TimothyGu TimothyGu Jun 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather leave that to a separate PR is what I meant.

v8::ConstructorBehavior::kAllow,
side_effect_type)->GetFunction();
// kInternalized strings are created in the old space.
const v8::NewStringType type = v8::NewStringType::kInternalized;
v8::Local<v8::String> name_string =
Expand All @@ -696,10 +703,12 @@ inline void Environment::SetMethod(v8::Local<v8::Object> that,

inline void Environment::SetProtoMethod(v8::Local<v8::FunctionTemplate> that,
const char* name,
v8::FunctionCallback callback) {
v8::FunctionCallback callback,
v8::SideEffectType side_effect_type) {
v8::Local<v8::Signature> signature = v8::Signature::New(isolate(), that);
v8::Local<v8::FunctionTemplate> t =
NewFunctionTemplate(callback, signature, v8::ConstructorBehavior::kThrow);
NewFunctionTemplate(callback, signature, v8::ConstructorBehavior::kThrow,
side_effect_type);
// kInternalized strings are created in the old space.
const v8::NewStringType type = v8::NewStringType::kInternalized;
v8::Local<v8::String> name_string =
Expand All @@ -708,10 +717,14 @@ inline void Environment::SetProtoMethod(v8::Local<v8::FunctionTemplate> that,
t->SetClassName(name_string); // NODE_SET_PROTOTYPE_METHOD() compatibility.
}

inline void Environment::SetTemplateMethod(v8::Local<v8::FunctionTemplate> that,
const char* name,
v8::FunctionCallback callback) {
v8::Local<v8::FunctionTemplate> t = NewFunctionTemplate(callback);
inline void Environment::SetTemplateMethod(
v8::Local<v8::FunctionTemplate> that,
const char* name,
v8::FunctionCallback callback,
v8::SideEffectType side_effect_type) {
v8::Local<v8::FunctionTemplate> t =
NewFunctionTemplate(callback, v8::Local<v8::Signature>(),
v8::ConstructorBehavior::kAllow, side_effect_type);
// kInternalized strings are created in the old space.
const v8::NewStringType type = v8::NewStringType::kInternalized;
v8::Local<v8::String> name_string =
Expand Down
16 changes: 12 additions & 4 deletions src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -752,18 +752,26 @@ class Environment {
v8::Local<v8::Signature> signature =
v8::Local<v8::Signature>(),
v8::ConstructorBehavior behavior =
v8::ConstructorBehavior::kAllow);
v8::ConstructorBehavior::kAllow,
v8::SideEffectType side_effect_type =
v8::SideEffectType::kHasSideEffect);

// Convenience methods for NewFunctionTemplate().
inline void SetMethod(v8::Local<v8::Object> that,
const char* name,
v8::FunctionCallback callback);
v8::FunctionCallback callback,
v8::SideEffectType side_effect_type =
v8::SideEffectType::kHasSideEffect);
inline void SetProtoMethod(v8::Local<v8::FunctionTemplate> that,
const char* name,
v8::FunctionCallback callback);
v8::FunctionCallback callback,
v8::SideEffectType side_effect_type =
v8::SideEffectType::kHasSideEffect);
inline void SetTemplateMethod(v8::Local<v8::FunctionTemplate> that,
const char* name,
v8::FunctionCallback callback);
v8::FunctionCallback callback,
v8::SideEffectType side_effect_type =
v8::SideEffectType::kHasSideEffect);

void BeforeExit(void (*cb)(void* arg), void* arg);
void RunBeforeExitCallbacks();
Expand Down
7 changes: 5 additions & 2 deletions src/inspector_js_api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ using v8::Local;
using v8::MaybeLocal;
using v8::NewStringType;
using v8::Object;
using v8::SideEffectType;
using v8::String;
using v8::Value;

Expand Down Expand Up @@ -289,7 +290,8 @@ void Initialize(Local<Object> target, Local<Value> unused,
if (agent->IsWaitingForConnect())
env->SetMethod(target, "callAndPauseOnStart", CallAndPauseOnStart);
env->SetMethod(target, "open", Open);
env->SetMethod(target, "url", Url);
env->SetMethod(target, "url", Url,
SideEffectType::kHasNoSideEffect);

env->SetMethod(target, "asyncTaskScheduled", AsyncTaskScheduledWrapper);
env->SetMethod(target, "asyncTaskCanceled",
Expand All @@ -300,7 +302,8 @@ void Initialize(Local<Object> target, Local<Value> unused,
InvokeAsyncTaskFnWithId<&Agent::AsyncTaskFinished>);

env->SetMethod(target, "registerAsyncHook", RegisterAsyncHookWrapper);
env->SetMethod(target, "isEnabled", IsEnabled);
env->SetMethod(target, "isEnabled", IsEnabled,
SideEffectType::kHasNoSideEffect);

auto conn_str = FIXED_ONE_BYTE_STRING(env->isolate(), "Connection");
Local<FunctionTemplate> tmpl =
Expand Down
13 changes: 9 additions & 4 deletions src/module_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ using v8::Object;
using v8::Promise;
using v8::ScriptCompiler;
using v8::ScriptOrigin;
using v8::SideEffectType;
using v8::String;
using v8::TryCatch;
using v8::Undefined;
Expand Down Expand Up @@ -789,11 +790,15 @@ void ModuleWrap::Initialize(Local<Object> target,
env->SetProtoMethod(tpl, "link", Link);
env->SetProtoMethod(tpl, "instantiate", Instantiate);
env->SetProtoMethod(tpl, "evaluate", Evaluate);
env->SetProtoMethod(tpl, "namespace", Namespace);
env->SetProtoMethod(tpl, "getStatus", GetStatus);
env->SetProtoMethod(tpl, "getError", GetError);
env->SetProtoMethod(tpl, "namespace", Namespace,
SideEffectType::kHasNoSideEffect);
env->SetProtoMethod(tpl, "getStatus", GetStatus,
SideEffectType::kHasNoSideEffect);
env->SetProtoMethod(tpl, "getError", GetError,
SideEffectType::kHasNoSideEffect);
env->SetProtoMethod(tpl, "getStaticDependencySpecifiers",
GetStaticDependencySpecifiers);
GetStaticDependencySpecifiers,
SideEffectType::kHasNoSideEffect);

target->Set(FIXED_ONE_BYTE_STRING(isolate, "ModuleWrap"), tpl->GetFunction());
env->SetMethod(target, "resolve", Resolve);
Expand Down
27 changes: 19 additions & 8 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ using v8::Promise;
using v8::PropertyCallbackInfo;
using v8::ScriptOrigin;
using v8::SealHandleScope;
using v8::SideEffectType;
using v8::String;
using v8::TryCatch;
using v8::Undefined;
Expand Down Expand Up @@ -1947,7 +1948,10 @@ void SetupProcessObject(Environment* env,
title_string,
ProcessTitleGetter,
env->is_main_thread() ? ProcessTitleSetter : nullptr,
env->as_external()).FromJust());
env->as_external(),
v8::DEFAULT,
v8::None,
SideEffectType::kHasNoSideEffect).FromJust());

// process.version
READONLY_PROPERTY(process,
Expand Down Expand Up @@ -2252,17 +2256,24 @@ void SetupProcessObject(Environment* env,
env->SetMethod(process, "_getActiveHandles", GetActiveHandles);
env->SetMethod(process, "_kill", Kill);

env->SetMethod(process, "cwd", Cwd);
env->SetMethod(process, "cwd", Cwd,
SideEffectType::kHasNoSideEffect);
env->SetMethod(process, "dlopen", DLOpen);
env->SetMethod(process, "reallyExit", Exit);
env->SetMethod(process, "uptime", Uptime);
env->SetMethod(process, "uptime", Uptime,
SideEffectType::kHasNoSideEffect);

#if defined(__POSIX__) && !defined(__ANDROID__) && !defined(__CloudABI__)
env->SetMethod(process, "getuid", GetUid);
env->SetMethod(process, "geteuid", GetEUid);
env->SetMethod(process, "getgid", GetGid);
env->SetMethod(process, "getegid", GetEGid);
env->SetMethod(process, "getgroups", GetGroups);
env->SetMethod(process, "getuid", GetUid,
Copy link
Copy Markdown
Member

@jasnell jasnell Jun 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought... but a env->SetSafeMethod(process, ...) variant that sets SideEfectType::kHasNoSideEffect automatically may be worthwhile.

SideEffectType::kHasNoSideEffect);
env->SetMethod(process, "geteuid", GetEUid,
SideEffectType::kHasNoSideEffect);
env->SetMethod(process, "getgid", GetGid,
SideEffectType::kHasNoSideEffect);
env->SetMethod(process, "getegid", GetEGid,
SideEffectType::kHasNoSideEffect);
env->SetMethod(process, "getgroups", GetGroups,
SideEffectType::kHasNoSideEffect);
#endif // __POSIX__ && !defined(__ANDROID__) && !defined(__CloudABI__)
}

Expand Down
43 changes: 29 additions & 14 deletions src/node_buffer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ using v8::Local;
using v8::Maybe;
using v8::MaybeLocal;
using v8::Object;
using v8::SideEffectType;
using v8::String;
using v8::Uint32Array;
using v8::Uint8Array;
Expand Down Expand Up @@ -1083,12 +1084,18 @@ void SetupBufferJS(const FunctionCallbackInfo<Value>& args) {
Local<Object> proto = args[0].As<Object>();
env->set_buffer_prototype_object(proto);

env->SetMethod(proto, "asciiSlice", StringSlice<ASCII>);
env->SetMethod(proto, "base64Slice", StringSlice<BASE64>);
env->SetMethod(proto, "latin1Slice", StringSlice<LATIN1>);
env->SetMethod(proto, "hexSlice", StringSlice<HEX>);
env->SetMethod(proto, "ucs2Slice", StringSlice<UCS2>);
env->SetMethod(proto, "utf8Slice", StringSlice<UTF8>);
env->SetMethod(proto, "asciiSlice", StringSlice<ASCII>,
SideEffectType::kHasNoSideEffect);
env->SetMethod(proto, "base64Slice", StringSlice<BASE64>,
SideEffectType::kHasNoSideEffect);
env->SetMethod(proto, "latin1Slice", StringSlice<LATIN1>,
SideEffectType::kHasNoSideEffect);
env->SetMethod(proto, "hexSlice", StringSlice<HEX>,
SideEffectType::kHasNoSideEffect);
env->SetMethod(proto, "ucs2Slice", StringSlice<UCS2>,
SideEffectType::kHasNoSideEffect);
env->SetMethod(proto, "utf8Slice", StringSlice<UTF8>,
SideEffectType::kHasNoSideEffect);

env->SetMethod(proto, "asciiWrite", StringWrite<ASCII>);
env->SetMethod(proto, "base64Write", StringWrite<BASE64>);
Expand Down Expand Up @@ -1116,22 +1123,30 @@ void Initialize(Local<Object> target,
Environment* env = Environment::GetCurrent(context);

env->SetMethod(target, "setupBufferJS", SetupBufferJS);
env->SetMethod(target, "createFromString", CreateFromString);
env->SetMethod(target, "createFromString", CreateFromString,
SideEffectType::kHasNoSideEffect);

env->SetMethod(target, "byteLengthUtf8", ByteLengthUtf8);
env->SetMethod(target, "byteLengthUtf8", ByteLengthUtf8,
SideEffectType::kHasNoSideEffect);
env->SetMethod(target, "copy", Copy);
env->SetMethod(target, "compare", Compare);
env->SetMethod(target, "compareOffset", CompareOffset);
env->SetMethod(target, "compare", Compare,
SideEffectType::kHasNoSideEffect);
env->SetMethod(target, "compareOffset", CompareOffset,
SideEffectType::kHasNoSideEffect);
env->SetMethod(target, "fill", Fill);
env->SetMethod(target, "indexOfBuffer", IndexOfBuffer);
env->SetMethod(target, "indexOfNumber", IndexOfNumber);
env->SetMethod(target, "indexOfString", IndexOfString);
env->SetMethod(target, "indexOfBuffer", IndexOfBuffer,
SideEffectType::kHasNoSideEffect);
env->SetMethod(target, "indexOfNumber", IndexOfNumber,
SideEffectType::kHasNoSideEffect);
env->SetMethod(target, "indexOfString", IndexOfString,
SideEffectType::kHasNoSideEffect);

env->SetMethod(target, "swap16", Swap16);
env->SetMethod(target, "swap32", Swap32);
env->SetMethod(target, "swap64", Swap64);

env->SetMethod(target, "encodeUtf8String", EncodeUtf8String);
env->SetMethod(target, "encodeUtf8String", EncodeUtf8String,
SideEffectType::kHasNoSideEffect);

target->Set(env->context(),
FIXED_ONE_BYTE_STRING(env->isolate(), "kMaxLength"),
Expand Down
Loading