Firstly, it also shuts down the warning when it turns into a *real* warning. For example this function will not produce a warning:

int test(int a) { int uninitialized_var(b);

return b; }

Secondly, if the *compiler* cannot understand the flow then the code is obviously rather complex for humans to review. So if there's an initialization bug in the future, the risk of a human not seeing it and the risk of uninitialized_var() hiding it is larger.

So the recommended thing is to simplify the flow there to make it easier for the compiler to see through it.

A quick look suggests that walk_addr_generic() is *horrible*: it has amassed a large number of classic code structure mistakes, and while it's clearly a performance critical function, needless code ugliness often goes at the *expense* of good performance.

I found a handful of problems during a quick review of it:

- There's ugly repeated patterns of:

if (unlikely(condition)) { present = false; break; }

which is then handled outside the main loop with:

if (unlikely(!present || ...)) goto error;

It would be a lot cleaner, not to mention faster as well to do this via:

if (condition) goto error_not_present;

That way the 'present' bool does not clog up the code flow (and register allocations).

- rsvd_fault shows similar mismanagement:

if (unlikely(condition)) { rsvd_fault = true; break; }

if (!eperm && !rsvd_fault && ...) { ... } }

if (unlikely(!present || eperm || rsvd_fault)) goto error;

This obfuscation complicated (and potentially slowed down) the middle condition: it's rather clear that the code flow cannot get there with rsvd == true ...

is idempotent so is an obvious candidate to be factored out into a helper inline. If you already know how eperm is calculated why should a code reader be forced to go through those lines again and again, every time this function is reviewed?

- In fact, once the unnecessary rsvd_fault complication has been factored out, the heart of the function, marking the pte accessed/dirty connects very nicely to the eperm calculating inline:

eperm = gpte_eperm(vcpu, pte, access);

[ NOTE: we should probably pass in 'access' explicitly because for code generation it's better to keep such variables in a single register and check it via the obvious bitmask and TESTL, not via the separate write_fault, user_fault, fetch_fault variables. ]

So ... we first split the 'access' attribute into 3 separate bools, then we *combine* them again and pass the result to translate_gpa()? Will the compiler figure out that this is equivalent to access & ~(PFERR_WRITE_MASK|PFERR_USER_MASK|PFERR_FETCH_MASK)?

Even if it does, wouldnt it be safe to pass 'access' to ->translate_gpa() as-is? If it's not safe to pass it as-is then a comment would be handy about this non-obvious looking fact.

- Variables are not marked 'const' where they should be - the above *_fault attributes for example but there are other examples as well. Since GCC very obviously has trouble seeing through this monster of a function, not helping it out with 'const' can hurt code generation quality. Reviewers are also helped: i had to spend a minute figuring out that none of these are ever modified within the function.

- What the heck is up with ASSERT() usage in the Linux kernel? arch/x86/kvm/ uses about 50% of BUG_ON()s and 50% of inverted logic ASSERT()s. If the goal was to confuse the reviewer then it's a full success! :-)

- Litte details like:

if (unlikely(kvm_is_error_hva(host_addr))) {

The name already suggests that kvm_is_error_hva() is a rare exception mechanism. The unlikely() could be propagated *into* kvm_is_error_hva() and thus call sites would be less cluttered.

- Data type choicese are sometimes unnatural and lead to unnecessary casts. For example:

- I'd suggest splitting the iterator of the loop out into a helper inline and only leave the loop / retry and error logic in walk_addr_generic(). Maybe even factor out the initialization and error logic - only leaving the main retry logic in walk_addr_generic() itself.

All in one, having spent a few minutes with this code i am not surprised *at all* that it has grown its *second* dangerous uninitialized_var() annotation ...