Skip to content
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.

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

v8o_optional serves no purpose #120

Open
mgerdts opened this issue Jun 4, 2019 · 0 comments
Open

v8o_optional serves no purpose #120

mgerdts opened this issue Jun 4, 2019 · 0 comments

Comments

@mgerdts
Copy link
Contributor

mgerdts commented Jun 4, 2019

mdb_v8.c has:

typedef struct v8_offset {
	ssize_t		*v8o_valp;
	const char	*v8o_class;
	const char	*v8o_member;
	boolean_t	v8o_optional;
	uint32_t	v8o_flags;
	intptr_t	v8o_fallback;
} v8_offset_t;

v8o_flags has:

#define	V8_CONSTANT_OPTIONAL		1
#define	V8_CONSTANT_HASFALLBACK		2
#define	V8_CONSTANT_REMOVED		4
#define	V8_CONSTANT_ADDED		8

V8_CONSTANT_FALLBACK() is commonly used to indicate that a value is optional and that it has a fallback value by setting V8_CONSTANT_OPTIONAL, among other things. However, v8o_optional is set to non-zero and V8_CONSTANT_FALLBACK() is used, the fallback value will never be used. This is confusing.

Worse, there are some offsets that are improperly initialized such that v8o_optional is set to a non-zero value intended for v8o_flags. A proper v8_offset_t initializer is:

	{ &V8_OFF_MAP_INOBJECT_PROPERTIES_OR_CTOR_FUN_INDEX,
	    "Map", "inobject_properties_or_constructor_function_index",
	    B_FALSE, V8_CONSTANT_FALLBACK(4, 6), 4 },

Examples of improper initializers that omit the (required) v8o_optional field are:

	{ &V8_OFF_SHAREDFUNCTIONINFO_INFERRED_NAME,
	    "SharedFunctionInfo", "inferred_name",
		V8_CONSTANT_REMOVED_SINCE(5, 1) },
#ifdef _LP64
	{ &V8_OFF_SHAREDFUNCTIONINFO_IDENTIFIER,
	    "SharedFunctionInfo", "function_identifier",
		V8_CONSTANT_FALLBACK(5, 1), 79},
#else
	{ &V8_OFF_SHAREDFUNCTIONINFO_IDENTIFIER,
	    "SharedFunctionInfo", "function_identifier",
		V8_CONSTANT_FALLBACK(5, 1), 39},
#endif

It is understandable that someone made this mistake. v8_constants[] uses very similar initializers but does not have an equivalent of the v8o_optional field.

Code inspection and testing suggests that v8o_optional should go away.

@mgerdts mgerdts self-assigned this Jun 4, 2019
mgerdts pushed a commit that referenced this issue Jun 4, 2019
@mgerdts mgerdts removed their assignment Dec 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant