Page MenuHomeFreeBSD

Avoid dump_avail[] redefinition.
ClosedPublic

Authored by kib on Oct 11 2020, 7:44 PM.

Details

Summary

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

This fixes default gcc build for mips.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib requested review of this revision.Oct 11 2020, 7:44 PM

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 ---

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.

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.

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.

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

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.

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 edited the summary of this revision. (Show Details)
kib added reviewers: markj, scottph, alc.

tb passes

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.

This revision is now accepted and ready to land.Oct 12 2020, 3:19 PM
sys/amd64/amd64/uma_machdep.c
45 ↗(On Diff #78128)

This is now redundant.

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

This revision now requires review to proceed.Oct 12 2020, 9:40 PM

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

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.

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

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 marked 9 inline comments as done.
kib edited the summary of this revision. (Show Details)

Remove redundant includes of machine/vmparam.h.

This revision is now accepted and ready to land.Oct 14 2020, 5:22 PM
This revision was automatically updated to reflect the committed changes.