Re: [GIT PULL] bcachefs

[Arnaldo Carvalho de Melo]

Em Wed, Sep 06, 2023 at 01:20:59PM -0700, Linus Torvalds escreveu:
> On Wed, 6 Sept 2023 at 13:02, Linus Torvalds
> <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > And guess what happens when you have (unsigned char)-1? It does *not*
> > cast back to -1.
> 
> Side note: again, this may be one of those "it works in practice",
> because if we have -fshort-enums, I think 'enum
> btree_node_locked_type' in turn ends up being represented as a 'signed
> char', because that's the smallest simple type that can fit all those
> values.
 
> I don't think gcc ever uses less than that (ie while a six_lock_type
> could fit in two bits, it's still going to be considered at least a
> 8-bit value in practice).

There are some cases where people stuff the enum into a bitfield, but
no, no simple type.

⬢[acme@toolbox perf-tools-next]$ pahole | grep -w enum | grep :
	enum btrfs_rsv_type        type:8;               /*    28:16  4 */
	enum btrfs_delayed_item_type type:8;             /*   100: 0  4 */
	enum kernel_pkey_operation op:8;                 /*    40: 0  4 */
	enum integrity_status      ima_file_status:4;    /*    96: 0  4 */
	enum integrity_status      ima_mmap_status:4;    /*    96: 4  4 */
	enum integrity_status      ima_bprm_status:4;    /*    96: 8  4 */
	enum integrity_status      ima_read_status:4;    /*    96:12  4 */
	enum integrity_status      ima_creds_status:4;   /*    96:16  4 */
	enum integrity_status      evm_status:4;         /*    96:20  4 */
	enum fs_context_purpose    purpose:8;            /*   152: 0  4 */
	enum fs_context_phase      phase:8;              /*   152: 8  4 */
	enum fs_value_type         type:8;               /*     8: 0  4 */
	enum sgx_page_type         type:16;              /*     8: 8  4 */
	enum nf_hook_ops_type      hook_ops_type:8;      /*    24: 8  4 */
		enum resctrl_event_id evtid:8;         /*     0:10  4 */
		enum _cache_type   type:5;             /*     0: 0  4 */
⬢[acme@toolbox perf-tools-next]$ pahole _cache_type
enum _cache_type {
	CTYPE_NULL    = 0,
	CTYPE_DATA    = 1,
	CTYPE_INST    = 2,
	CTYPE_UNIFIED = 3,
};

⬢[acme@toolbox perf-tools-next]$ pahole integrity_status
enum integrity_status {
	INTEGRITY_PASS           = 0,
	INTEGRITY_PASS_IMMUTABLE = 1,
	INTEGRITY_FAIL           = 2,
	INTEGRITY_FAIL_IMMUTABLE = 3,
	INTEGRITY_NOLABEL        = 4,
	INTEGRITY_NOXATTRS       = 5,
	INTEGRITY_UNKNOWN        = 6,
};

⬢[acme@toolbox perf-tools-next]
> 
> So we may have 'enum six_lock_type' essentially being 'unsigned char',
> and when the code does
> 
>     mark_btree_node_locked(trans, path, 0, BTREE_NODE_UNLOCKED);
> 
> that BTREE_NODE_UNLOCKED value might actually be 255.
> 
> And then when it's cast to 'enum btree_node_locked_type' in the inline
> function, the 255 will be cast to 'signed char', and we'll end up
> compatible with '(enum btree_node_locked_type)-1' again.
> 
> So it's one of those things that are seriously wrong to do, but might
> generate the expected code anyway.
> 
> Unless the compiler adds any other sanity checks, like UBSAN or
> something, that actually uses the exact range of the enums.
> 
> It could happen even without UBSAN, if the compiler ends up going "I
> can see that the original value came from a 'enum six_lock_type', so I
> know the original value can't be signed, so any comparison with
> BTREE_NODE_UNLOCKED can never be true.
> 
> But again, I suspect that in practice this all just happens to work.
> That doesn't make it right.
> 
>                Linus

-- 

- Arnaldo