Generating derives for Hash, Default and Debug for an Union

[raphaelcohn]

Derives Hash, Default and Debug for an Union, which won't compile: error: this trait cannot be derived for unions.

Input C/C++ Header

struct mpw_data {
	uint8_t		state; /* use mpw_states */
	uint8_t		size;
	uint8_t		num_sge;
	uint32_t	len;
	uint32_t	total_len;
	uint32_t	flags;
	uint32_t	scur_post;
	union {
		struct mlx5_wqe_data_seg	*last_dseg;
		uint8_t				*inl_data;
	};
	uint32_t			*ctrl_update;
};

Bindgen Invocation

bindgen --verbose \
		--disable-name-namespacing --no-doc-comments --no-layout-tests --no-recursive-whitelist --objc-extern-crate \
		--rustfmt-configuration-file "$_program_path"/rustfmt.toml \
		--rust-target nightly \
		--impl-debug --impl-partialeq \
		--with-derive-default --with-derive-eq --with-derive-hash --with-derive-ord \
		--ctypes-prefix ::libc --output "$outputFilePath" ...

Actual Results

#[repr(C)]
#[derive(Debug, Default, Copy, Clone, Hash)]
pub union mpw_data__bindgen_ty_1
{
	pub last_dseg: *mut mlx5_wqe_data_seg,
	pub inl_data: *mut u8,
	_bindgen_union_align: u64,
	pub _address: u8,
}

[emilio]

This should be trivial to fix btw, see src/ir/analysis/derive_hash.rs for example.

I'm happy to mentor this if you'r blocked on this or what not.

[marty-se]

Rust newcomer here, but I was bitten by this bug in my first bindgen test. :) Anyways, I decided to give it a debug shot.

I think the bug is related to whitelisting combined with anonymous unions, and I suspect the bug is in the sub_item whitelist test in generate_dependencies(). It checks whether each sub_item is whitelisted or not, and if the sub_item isn't whitelisted (even if it's an anonymous union) it won't eventually be added to the worklist.

I'm not sure if the correct fix is to make sure anonymous compounds are always whitelisted, or if the test in generate_dependencies() should check if the sub_item is anonymous.

[marty-se]

Btw, I wouldn’t mind continuing working on this if it isn’t too complicated to fix, but I’d need some advice on the question above.

[marty-se]

It seems inner types need to be whitelisted even when --no-recursive-whitelisting is specified.

In addition to the inner types being missed by generate_dependencies (which seems to be what causes the original issue), I also noticed that codegen complained that Found non-whitelisted item in code generation.

I made an attempt to fix this on my fork, along with a test case. Should I file a PR or would perhaps the maintainers like to make an architectural review first (@emilio ?) I don't really feel like I have the big picture of everything... :)

[emilio]

Hmm, ok, I see what happens.

So what's going on is that we generate the inner types unconditionally if the outer type is whitelisted... Maybe we should just avoid blacklisting inner types in the first place? Given it's effectively not honored anyway.

In any case, that seems like a reasonable fix to me, modulo a minor nit (I'd just replace the function body with edge.kind == EdgeKind::InnerType, and maybe document it more thoroughly to understand why is it needed). Probably worth getting reviewed by @fitzgen too...

Basically in order to compute derive(..) bounds we need to always account for inner types, even if they're not whitelisted. The derive(..) stuff seems kinda fragile without recursive whitelisting, in any case...

[marty-se]

Thanks for your feedback. I've added some more doc and renamed the predicate for clarity. Please see my pull request.

[raphaelcohn]

@emilio thanks for offering to mentor this, but I'm rather busy. I'm glad @marty-se has stepped in. Thanks! (As an aside, I get around these blockers by running bindgen inside a shell script bindgen-wrapper that lets me post-process stuff like this).

[emilio]

This was fixed by #1294.