nodejs / llnode

An lldb plugin for Node.js and V8, which enables inspection of JavaScript states for insights into Node.js processes and their core dumps.
Other
1.15k stars 99 forks source link

Wrong instance_size? #333

Closed hyj1991 closed 4 years ago

hyj1991 commented 4 years ago

https://github.com/nodejs/llnode/blob/master/src/printer.cc#L489

If the map here is JS_MAP_TYPE, I think maybe the instance_size should be:

int64_t instance_size;
if (map.IsJSObjectMap(err)) {
  v8::HeapObject map_object = map.GetMap(err);
  v8::Map map(map_object);
  instance_size = map.InstanceSize(err);
} else {
  instance_size = map.InstanceSize(err);
}
if (err.Fail()) return std::string();
mmarchini commented 4 years ago

When the code gets here it should already be a v8::Map, but maybe we're missing some corner case here. JS_MAP_TYPE should fall under https://github.com/nodejs/llnode/blob/master/src/printer.cc#L655. Did you hit an issue with it? If so, can you share a small snippet with a reproducible (or more info)?

(JS_MAP_TYPE is https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map, right? If so, it's probably worth having a class dedicated for it)

hyj1991 commented 4 years ago

@mmarchini Thanks for your reply :)

When the code gets here it should already be a v8::Map, but maybe we're missing some corner case here. JS_MAP_TYPE should fall under https://github.com/nodejs/llnode/blob/master/src/printer.cc#L655.

Indeed so. But if it already is a v8::Map, why should we judge here whether it is a js object map: https://github.com/nodejs/llnode/blob/master/src/printer.cc#L478

inline bool Map::IsJSObjectMap(Error& err) {
  return InstanceType(err) >= v8()->types()->kFirstJSObjectType;
}

kFirstJSObjectType is v8dbg_type_JSGlobalObject__JS_GLOBAL_OBJECT_TYPE (in reality JS_GLOBAL_OBJECT_TYPE), and I see JS_MAP_TYPE (src/objects/instance-type.h) is larger than JS_GLOBAL_OBJECT_TYPE, that's why I confused here.

(JS_MAP_TYPE is https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map, right? If so, it's probably worth having a class dedicated for it)

Indeed so.

mmarchini commented 4 years ago

I'll have to take a closer look to answer your questions (I'm going on vacation today, so I'll try to answer it when I'm back around late-Feb)

hyj1991 commented 4 years ago

Thanks a lot :)