Change in coreboot[master]: [WIP] mb/emulation/qemu-i440fx: Remove TRACE=y from test build
Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43817 )
Change subject: [WIP] mb/emulation/qemu-i440fx: Remove TRACE=y from test build ......................................................................
[WIP] mb/emulation/qemu-i440fx: Remove TRACE=y from test build
Looks like the option is generally not compatible with garbage collections.
Change-Id: I1b76dc34b5f39d8988368f71a0a2f43d1bc4177e Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M configs/config.emulation_qemu_x86_i440fx_debug 1 file changed, 0 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/43817/1
diff --git a/configs/config.emulation_qemu_x86_i440fx_debug b/configs/config.emulation_qemu_x86_i440fx_debug index 011f163..e39ff59 100644 --- a/configs/config.emulation_qemu_x86_i440fx_debug +++ b/configs/config.emulation_qemu_x86_i440fx_debug @@ -4,6 +4,5 @@ CONFIG_DEBUG_CBFS=y CONFIG_DEBUG_PIRQ=y CONFIG_DEBUG_MALLOC=y -CONFIG_TRACE=y CONFIG_DEBUG_BOOT_STATE=y CONFIG_DEBUG_ADA_CODE=y
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43817 )
Change subject: [WIP] mb/emulation/qemu-i440fx: Remove TRACE=y from test build ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43817/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43817/1//COMMIT_MSG@10 PS1, Line 10: garbage collections. I don't follow. Garbage collections?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43817 )
Change subject: [WIP] mb/emulation/qemu-i440fx: Remove TRACE=y from test build ......................................................................
Patch Set 1:
(1 comment)
See build er https://qa.coreboot.org/job/coreboot-gerrit/136133/testReport/junit/(root)/b...
https://review.coreboot.org/c/coreboot/+/43817/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43817/1//COMMIT_MSG@10 PS1, Line 10: garbage collections.
I don't follow. […]
See the build error in parent that this fixes.
https://qa.coreboot.org/job/coreboot-gerrit/136133/testReport/junit/(root)/b...
Looks like compiler does not optimise is_smp_boot() to a constant false with TRACE=y, and copy_secondary_start_to_lowest_1M() is kept in the resulting object. We have CONFIG_SMP guard for secondary.S and those symbols now became unresolved.
OTH, if (ENV_xx) and if (CONFIG(x)) should not be affected by TRACE so it might be just a couple special cases where it fails like seen here.
Hello build bot (Jenkins), Nico Huber, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43817
to look at the new patch set (#2).
Change subject: mb/emulation/qemu-i440fx: Remove TRACE=y from test build ......................................................................
mb/emulation/qemu-i440fx: Remove TRACE=y from test build
Looks like the option is generally not compatible with garbage collections. Nothing is inlined, is_smp_boot() no longer evaluates to constant false and thus the symbols from secondary.S would need to be present for the build to pass after we set SMP=n.
Change-Id: I1b76dc34b5f39d8988368f71a0a2f43d1bc4177e Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M configs/config.emulation_qemu_x86_i440fx_debug 1 file changed, 0 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/43817/2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43817 )
Change subject: mb/emulation/qemu-i440fx: Remove TRACE=y from test build ......................................................................
Patch Set 2: Code-Review-1
(1 comment)
The `TRACE=y` is there so the option gets build tested. If removed for this board, it should at least be set for another. But generally, we build test so things can get fixed and not worked around. Maybe somebody will want to try it with QEMU? and then it doesn't build... that's what we actually want to avoid.
https://review.coreboot.org/c/coreboot/+/43817/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43817/2//COMMIT_MSG@11 PS2, Line 11: no longer evaluates to constant false Should the build rely on such functions, though? Or should the if be `(CONFIG(SMP) && is_smp_boot()` instead?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43817 )
Change subject: mb/emulation/qemu-i440fx: Remove TRACE=y from test build ......................................................................
Patch Set 2:
Patch Set 2: Code-Review-1
(1 comment)
The `TRACE=y` is there so the option gets build tested. If removed for this board, it should at least be set for another. But generally, we build test so things can get fixed and not worked around. Maybe somebody will want to try it with QEMU? and then it doesn't build... that's what we actually want to avoid.
Ever since RELOCATABLE_RAMSTAGE, TRACE=y has not made much sense, the pointers returned to console are not normalized. Looks like 8 years of nobody really caring about this.
Let's see how many boards actually build with it enabled.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43817 )
Change subject: mb/emulation/qemu-i440fx: Remove TRACE=y from test build ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43817/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43817/2//COMMIT_MSG@11 PS2, Line 11: no longer evaluates to constant false
Should the build rely on such functions, though? Or should […]
https://qa.coreboot.org/job/coreboot-gerrit/136372/#showFailuresLink
We have errors with SMP,VBOOT,DEBUG_SMI,TPM. I did not go through the list of 261 failed builds.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43817 )
Change subject: mb/emulation/qemu-i440fx: Remove TRACE=y from test build ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43817/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43817/2//COMMIT_MSG@11 PS2, Line 11: no longer evaluates to constant false
https://qa.coreboot.org/job/coreboot-gerrit/136372/#showFailuresLink […]
I suggest to drop it then (the whole feature). Should we bring it up on the ML?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43817 )
Change subject: mb/emulation/qemu-i440fx: Remove TRACE=y from test build ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43817/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43817/2//COMMIT_MSG@11 PS2, Line 11: no longer evaluates to constant false
I suggest to drop it then (the whole feature). […]
I would bring this up on the ML: "hey, this is broken with several common Kconfig options, shall we drop it or fix it?"
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43817 )
Change subject: mb/emulation/qemu-i440fx: Remove TRACE=y from test build ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43817/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43817/2//COMMIT_MSG@11 PS2, Line 11: no longer evaluates to constant false
I would bring this up on the ML: "hey, this is broken with several common Kconfig options, shall we […]
Done
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/43817 )
Change subject: mb/emulation/qemu-i440fx: Remove TRACE=y from test build ......................................................................
Abandoned
Kyösti Mälkki has restored this change. ( https://review.coreboot.org/c/coreboot/+/43817 )
Change subject: mb/emulation/qemu-i440fx: Remove TRACE=y from test build ......................................................................
Restored
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43817 )
Change subject: mb/emulation/qemu-i440fx: Remove TRACE=y from test build ......................................................................
Patch Set 4: Code-Review+2
Nobody seems to have an interest to maintain it. So we definitely don't need to built test something that only compiles by accident.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43817 )
Change subject: mb/emulation/qemu-i440fx: Remove TRACE=y from test build ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43817/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43817/1//COMMIT_MSG@10 PS1, Line 10: garbage collections.
See the build error in parent that this fixes. […]
Done, I suppose. Angel?
https://review.coreboot.org/c/coreboot/+/43817/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43817/2//COMMIT_MSG@11 PS2, Line 11: no longer evaluates to constant false
Done
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43817 )
Change subject: mb/emulation/qemu-i440fx: Remove TRACE=y from test build ......................................................................
Patch Set 4: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/43817/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43817/1//COMMIT_MSG@10 PS1, Line 10: garbage collections.
Done, I suppose. […]
Done
https://review.coreboot.org/c/coreboot/+/43817/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43817/2//COMMIT_MSG@11 PS2, Line 11: no longer evaluates to constant false
Done
Done
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43817 )
Change subject: mb/emulation/qemu-i440fx: Remove TRACE=y from test build ......................................................................
mb/emulation/qemu-i440fx: Remove TRACE=y from test build
Looks like the option is generally not compatible with garbage collections. Nothing is inlined, is_smp_boot() no longer evaluates to constant false and thus the symbols from secondary.S would need to be present for the build to pass after we set SMP=n.
Change-Id: I1b76dc34b5f39d8988368f71a0a2f43d1bc4177e Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/43817 Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M configs/config.emulation_qemu_x86_i440fx_debug 1 file changed, 0 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/configs/config.emulation_qemu_x86_i440fx_debug b/configs/config.emulation_qemu_x86_i440fx_debug index 011f163..e39ff59 100644 --- a/configs/config.emulation_qemu_x86_i440fx_debug +++ b/configs/config.emulation_qemu_x86_i440fx_debug @@ -4,6 +4,5 @@ CONFIG_DEBUG_CBFS=y CONFIG_DEBUG_PIRQ=y CONFIG_DEBUG_MALLOC=y -CONFIG_TRACE=y CONFIG_DEBUG_BOOT_STATE=y CONFIG_DEBUG_ADA_CODE=y
-
Angel Pons (Code Review) -
Kyösti Mälkki (Code Review)
-
Nico Huber (Code Review)