Avoid dump_avail[] redefinition.

[kib]

Summary

Move dump_avail[] extern declaration and inlines into new header vm/vm_dumpset.h.

This fixes default gcc build for mips.

[kevans]

This seems to remedy the initial problem. I then ran into a 'function with a non-void return type does not return a value' in @mjg's work and applied this diff because it looks like the switch is supposed to cover it:

diff --git a/sys/kern/vfs_cache.c b/sys/kern/vfs_cache.c
index 0880ed26b28..e425dd85468 100644
--- a/sys/kern/vfs_cache.c
+++ b/sys/kern/vfs_cache.c
@@ -4242,6 +4242,8 @@ cache_fplookup_impl(struct vnode *dvp, struct cache_fpl *fpl)
                        cache_fpl_cleanup_cnp(cnp);
                return (error);
        }
+
+       __assert_unreachable();
 }
 
 /*

I then ran into this, which seems to make little sense:

/usr/src/sys/amd64/amd64/exception.S: Assembler messages:          
/usr/src/sys/amd64/amd64/exception.S:613: Error: found 'U', expected: ')'
/usr/src/sys/amd64/amd64/exception.S:613: Error: junk `UL)' after expression
/usr/src/sys/amd64/amd64/exception.S:1268: Error: found 'U', expected: ')'
/usr/src/sys/amd64/amd64/exception.S:1268: Error: junk `UL)' after expression                      
--- exception.o ---

[mjg]

Is this a gcc build?

gcc is notorious for the following problem: explicitly handing all enum cases one by one without any form of catch-all results in a warning that something is not handled. They have a bug report somewhere about this but they don't fix the problem.

The particular instance you see here is one of several, you did not see more because of the support.S compilation failure.

I don't know what's the best way to address this, given __address_unreachable semantics it is worrisome the compiler will assume the not mentioned cases now got handling. I would argue the thing to do is to disable the enumeration warn to begin with.

[kevans]

Yeah, this is with amd64-gcc9 in my particular case, I was trying to test if bfd still "optimizes" away dynamic mode if there are no dynamic references.

[kib]

In D26741#596164, @kevans wrote:

I then ran into this, which seems to make little sense:

/usr/src/sys/amd64/amd64/exception.S: Assembler messages:          
/usr/src/sys/amd64/amd64/exception.S:613: Error: found 'U', expected: ')'
/usr/src/sys/amd64/amd64/exception.S:613: Error: junk `UL)' after expression
/usr/src/sys/amd64/amd64/exception.S:1268: Error: found 'U', expected: ')'
/usr/src/sys/amd64/amd64/exception.S:1268: Error: junk `UL)' after expression                      
--- exception.o ---

Look at assym.inc for PMAP_UCR3_NOMASK and UCR3_LOAD_MASK. Or better, pre-process exception.S and look at the preprocessed asm.

[mjg]

If someone is to do anything serious with gcc I suggest the following:

• scratch version 9, use 10. the port may need updating.
• review all passed flags. I know some warns got silenced over the years just to get the compile rolling, despite pointing at legitimate problems.
• find a switch to make gcc leave enums alone and rely on clang for correctness

The bug report I mentioned: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87950

[kib]

In D26741#596184, @mjg wrote:

If someone is to do anything serious with gcc I suggest the following:

• scratch version 9, use 10. the port may need updating.
• review all passed flags. I know some warns got silenced over the years just to get the compile rolling, despite pointing at legitimate problems.
• find a switch to make gcc leave enums alone and rely on clang for correctness

This is rather orthogonal to the review as is. The goal is to fix _mips_ gcc build, amd64 gcc would be nice and I will do whatever is needed. But in the different changeset.

[kevans]

Sorry, my commentary was also mostly orthogonal... I can confirm that this review + the vfs_cache.c hack are sufficient for mips buildkernel w/ gcc9.

[kib]

tb passes

[markj]

Could you update .clang-format to sort vm_param.h after vm.h and before vm/*?

sys/vm/vm_page.h

593 ↗

(On Diff #78128)

Are there many files that include vm_page.h without vm_param.h? It would be nicer to avoid the ifdef if possible.

[alc]

sys/amd64/amd64/uma_machdep.c

45 ↗

(On Diff #78128)

This is now redundant.

[alc]

Alternatively, move this code to a different header file. None of the new dump-related code uses struct vm_page.

[kib]

Add vm/vm_dumpset.h.

[scottph]

Thanks for fixing this. Grepping around, I believe vm_dumpset.h should also be included in:

sys/mips/atheros/ar531x/ar5315_machdep.c
sys/mips/broadcom/bcm_machdep.c
sys/mips/ingenic/jz4780_machdep.c
sys/mips/mediatek/mtk_machdep.c
sys/vm/vm_phys.c

[kib]

In D26741#596875, @scottph wrote:

Thanks for fixing this. Grepping around, I believe vm_dumpset.h should also be included in:

sys/mips/atheros/ar531x/ar5315_machdep.c
sys/mips/broadcom/bcm_machdep.c
sys/mips/ingenic/jz4780_machdep.c
sys/mips/mediatek/mtk_machdep.c
sys/vm/vm_phys.c

For vm_phys.c dump_avail[] is locally defined, I am not sure declaration is useful. It would provide some consistency checking indeed but contamination with unneeded symbols is probably worse than this minor improvement.

Other files you listed are not used during tinderbox builds. I added the header following your notes.

[kib]

Add vm_dumpset.h to mips machdep files not checked during tinderbox.

[alc]

sys/arm/arm/mem.c

73 ↗

(On Diff #78196)

This is redundant.

sys/arm64/arm64/uma_machdep.c

44 ↗

(On Diff #78196)

This is redundant.

sys/i386/i386/minidump_machdep_base.c

51 ↗

(On Diff #78196)

This is redundant.

sys/mips/mips/minidump_machdep.c

53 ↗

(On Diff #78196)

This is redundant.

sys/mips/mips/uma_machdep.c

46 ↗

(On Diff #78196)

This is redundant.

sys/powerpc/powerpc/uma_machdep.c

49 ↗

(On Diff #78196)

This is redundant.

sys/riscv/riscv/uma_machdep.c

44 ↗

(On Diff #78196)

This is redundant.

[kib]

Remove redundant includes of machine/vmparam.h.