objtool is throwing *unannotated intra-function call*
warnings with a few instructions that are marked
unreachable. Replace unreachable() with __builtin_unreachable()
to fix these warnings, as the codegen remains same
with unreachable() and __builtin_unreachable().
Signed-off-by: Sathvika Vasireddy <[email protected]>
---
arch/powerpc/include/asm/bug.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index 61a4736355c2..074be1a78c56 100644
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -99,7 +99,7 @@
__label__ __label_warn_on; \
\
WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags), __label_warn_on); \
- unreachable(); \
+ __builtin_unreachable(); \
\
__label_warn_on: \
break; \
--
2.31.1
Le 08/08/2022 à 13:48, Sathvika Vasireddy a écrit :
> objtool is throwing *unannotated intra-function call*
> warnings with a few instructions that are marked
> unreachable. Replace unreachable() with __builtin_unreachable()
> to fix these warnings, as the codegen remains same
> with unreachable() and __builtin_unreachable().
I think it is necessary to explain why using unreachable() is not
necessary for powerpc, or even why using unreachable() is wrong.
Allthough we are getting rid of the problem here by replacing
unreachable() by __builtin_unreachable(), it might still be a problem in
core parts of kernel which still use unreachable.
So maybe it would be better to leave that as is, and instead modify
annotate_unreachable() so that only architectures requiring it
absolutely get it. For instance by defining and using a new config item,
for instance CONFIG_OBJTOOL_NEEDS_ANNOTATE_UNREACHABLE
>
> Signed-off-by: Sathvika Vasireddy <[email protected]>
> ---
> arch/powerpc/include/asm/bug.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
> index 61a4736355c2..074be1a78c56 100644
> --- a/arch/powerpc/include/asm/bug.h
> +++ b/arch/powerpc/include/asm/bug.h
> @@ -99,7 +99,7 @@
> __label__ __label_warn_on; \
> \
> WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags), __label_warn_on); \
> - unreachable(); \
> + __builtin_unreachable(); \
> \
> __label_warn_on: \
> break; \
Christophe Leroy wrote:
>
>
> Le 08/08/2022 à 13:48, Sathvika Vasireddy a écrit :
>> objtool is throwing *unannotated intra-function call*
>> warnings with a few instructions that are marked
>> unreachable. Replace unreachable() with __builtin_unreachable()
>> to fix these warnings, as the codegen remains same
>> with unreachable() and __builtin_unreachable().
>
> I think it is necessary to explain why using unreachable() is not
> necessary for powerpc, or even why using unreachable() is wrong.
>
> Allthough we are getting rid of the problem here by replacing
> unreachable() by __builtin_unreachable(), it might still be a problem in
> core parts of kernel which still use unreachable.
I did a kernel build with this series applied, with a variant of
ppc64le_defconfig. I then did another build with the same config, but
with the below hunk to disable objtool:
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 6be2e68fa9eb64..4c466acdc70d4c 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -237,8 +237,6 @@ config PPC
select HAVE_MOD_ARCH_SPECIFIC
select HAVE_NMI if PERF_EVENTS || (PPC64 && PPC_BOOK3S)
select HAVE_OPTPROBES
- select HAVE_OBJTOOL if PPC32 || MPROFILE_KERNEL
- select HAVE_OBJTOOL_MCOUNT if HAVE_OBJTOOL
select HAVE_PERF_EVENTS
select HAVE_PERF_EVENTS_NMI if PPC64
select HAVE_PERF_REGS
This has the effect of disabling annotations for unreachable().
When I compared the resulting object files, I did not see changes in
codegen relating to the annotation, like we do with using unreachable()
in __WARN_FLAGS().
More specifically, arch/powerpc/kvm/book3s.o:kvmppc_h_logical_ci_load()
uses BUG(), and the generated code remains the same with/without the
unreachable() annotation.
This suggests that the bad codegen we are seeing with the annotation in
unreachable() is limited to its use in __WARN_FLAGS(), which I suspect
is due to an interaction with the use of asm_volatile_goto() for
WARN_ENTRY().
If I revert this patch (patch 01/16), gcc seems to add a label 8 bytes
before _some_ function in this object file, which happens to hold a
relocation against .TOC., and emits a bl to that symbol. Otherwise, gcc
either emits no new instruction for the annotation, or a 'nop' in some
cases.
If I add a 'nop' between WARN_ENTRY() and unreachable() in
__WARN_FLAGS(), or convert WARN_ENTRY to BUG_ENTRY thereby removing use
of asm_volatile_goto(), the problem goes away and no bl is emitted:
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index 61a4736355c244..88e0027c20ba5c 100644
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -99,6 +99,7 @@
__label__ __label_warn_on; \
\
WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags), __label_warn_on); \
+ __asm__ __volatile__("nop"); \
unreachable(); \
\
__label_warn_on:
In summary, I think the annotation itself is fine and we are only seeing
an issue with its usage after WARN_ENTRY() due to use of
asm_volatile_goto. Other uses of unreachable() don't seem to exhibit
this problem.
As such, I think this patch is appropriate for this series, though I
think we should capture some of this information in the changelog.
Note also that if and when we start utlizing the annotation, if we
classify twui as INSN_BUG, this change will continue to be appropriate.
- Naveen
Le 18/08/2022 à 12:46, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>>
>>
>> Le 08/08/2022 à 13:48, Sathvika Vasireddy a écrit :
>>> objtool is throwing *unannotated intra-function call*
>>> warnings with a few instructions that are marked
>>> unreachable. Replace unreachable() with __builtin_unreachable()
>>> to fix these warnings, as the codegen remains same
>>> with unreachable() and __builtin_unreachable().
>>
>> I think it is necessary to explain why using unreachable() is not
>> necessary for powerpc, or even why using unreachable() is wrong.
>>
>> Allthough we are getting rid of the problem here by replacing
>> unreachable() by __builtin_unreachable(), it might still be a problem
>> in core parts of kernel which still use unreachable.
>
> I did a kernel build with this series applied, with a variant of
> ppc64le_defconfig. I then did another build with the same config, but
> with the below hunk to disable objtool:
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 6be2e68fa9eb64..4c466acdc70d4c 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -237,8 +237,6 @@ config PPC
> select HAVE_MOD_ARCH_SPECIFIC
> select HAVE_NMI if PERF_EVENTS || (PPC64
> && PPC_BOOK3S)
> select HAVE_OPTPROBES
> - select HAVE_OBJTOOL if PPC32 || MPROFILE_KERNEL
> - select HAVE_OBJTOOL_MCOUNT if HAVE_OBJTOOL
> select HAVE_PERF_EVENTS
> select HAVE_PERF_EVENTS_NMI if PPC64
> select HAVE_PERF_REGS
>
> This has the effect of disabling annotations for unreachable().
>
> When I compared the resulting object files, I did not see changes in
> codegen relating to the annotation, like we do with using unreachable()
> in __WARN_FLAGS().
>
> More specifically, arch/powerpc/kvm/book3s.o:kvmppc_h_logical_ci_load()
> uses BUG(), and the generated code remains the same with/without the
> unreachable() annotation.
>
> This suggests that the bad codegen we are seeing with the annotation in
> unreachable() is limited to its use in __WARN_FLAGS(), which I suspect
> is due to an interaction with the use of asm_volatile_goto() for
> WARN_ENTRY().
>
> If I revert this patch (patch 01/16), gcc seems to add a label 8 bytes
> before _some_ function in this object file, which happens to hold a
> relocation against .TOC., and emits a bl to that symbol. Otherwise, gcc
> either emits no new instruction for the annotation, or a 'nop' in some
> cases.
>
> If I add a 'nop' between WARN_ENTRY() and unreachable() in
> __WARN_FLAGS(), or convert WARN_ENTRY to BUG_ENTRY thereby removing use
> of asm_volatile_goto(), the problem goes away and no bl is emitted:
>
> diff --git a/arch/powerpc/include/asm/bug.h
> b/arch/powerpc/include/asm/bug.h
> index 61a4736355c244..88e0027c20ba5c 100644
> --- a/arch/powerpc/include/asm/bug.h
> +++ b/arch/powerpc/include/asm/bug.h
> @@ -99,6 +99,7 @@
> __label__ __label_warn_on; \
> \
> WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags),
> __label_warn_on); \
> + __asm__ __volatile__("nop"); \
> unreachable(); \
> \
> __label_warn_on:
>
>
> In summary, I think the annotation itself is fine and we are only seeing
> an issue with its usage after WARN_ENTRY() due to use of
> asm_volatile_goto. Other uses of unreachable() don't seem to exhibit
> this problem.
>
> As such, I think this patch is appropriate for this series, though I
> think we should capture some of this information in the changelog.
>
> Note also that if and when we start utlizing the annotation, if we
> classify twui as INSN_BUG, this change will continue to be appropriate.
>
INSN_TRAP instead of INSN_BUG ?
Christophe
Christophe Leroy wrote:
>
>
> Le 18/08/2022 à 12:46, Naveen N. Rao a écrit :
>> Christophe Leroy wrote:
>>>
>>>
>>> Le 08/08/2022 à 13:48, Sathvika Vasireddy a écrit :
>>>> objtool is throwing *unannotated intra-function call*
>>>> warnings with a few instructions that are marked
>>>> unreachable. Replace unreachable() with __builtin_unreachable()
>>>> to fix these warnings, as the codegen remains same
>>>> with unreachable() and __builtin_unreachable().
>>>
>>> I think it is necessary to explain why using unreachable() is not
>>> necessary for powerpc, or even why using unreachable() is wrong.
>>>
>>> Allthough we are getting rid of the problem here by replacing
>>> unreachable() by __builtin_unreachable(), it might still be a problem
>>> in core parts of kernel which still use unreachable.
>>
>> I did a kernel build with this series applied, with a variant of
>> ppc64le_defconfig. I then did another build with the same config, but
>> with the below hunk to disable objtool:
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index 6be2e68fa9eb64..4c466acdc70d4c 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -237,8 +237,6 @@ config PPC
>> select HAVE_MOD_ARCH_SPECIFIC
>> select HAVE_NMI if PERF_EVENTS || (PPC64
>> && PPC_BOOK3S)
>> select HAVE_OPTPROBES
>> - select HAVE_OBJTOOL if PPC32 || MPROFILE_KERNEL
>> - select HAVE_OBJTOOL_MCOUNT if HAVE_OBJTOOL
>> select HAVE_PERF_EVENTS
>> select HAVE_PERF_EVENTS_NMI if PPC64
>> select HAVE_PERF_REGS
>>
>> This has the effect of disabling annotations for unreachable().
>>
>> When I compared the resulting object files, I did not see changes in
>> codegen relating to the annotation, like we do with using unreachable()
>> in __WARN_FLAGS().
>>
>> More specifically, arch/powerpc/kvm/book3s.o:kvmppc_h_logical_ci_load()
>> uses BUG(), and the generated code remains the same with/without the
>> unreachable() annotation.
>>
>> This suggests that the bad codegen we are seeing with the annotation in
>> unreachable() is limited to its use in __WARN_FLAGS(), which I suspect
>> is due to an interaction with the use of asm_volatile_goto() for
>> WARN_ENTRY().
>>
>> If I revert this patch (patch 01/16), gcc seems to add a label 8 bytes
>> before _some_ function in this object file, which happens to hold a
>> relocation against .TOC., and emits a bl to that symbol. Otherwise, gcc
>> either emits no new instruction for the annotation, or a 'nop' in some
>> cases.
>>
>> If I add a 'nop' between WARN_ENTRY() and unreachable() in
>> __WARN_FLAGS(), or convert WARN_ENTRY to BUG_ENTRY thereby removing use
>> of asm_volatile_goto(), the problem goes away and no bl is emitted:
>>
>> diff --git a/arch/powerpc/include/asm/bug.h
>> b/arch/powerpc/include/asm/bug.h
>> index 61a4736355c244..88e0027c20ba5c 100644
>> --- a/arch/powerpc/include/asm/bug.h
>> +++ b/arch/powerpc/include/asm/bug.h
>> @@ -99,6 +99,7 @@
>> __label__ __label_warn_on; \
>> \
>> WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags),
>> __label_warn_on); \
>> + __asm__ __volatile__("nop"); \
>> unreachable(); \
>> \
>> __label_warn_on:
>>
>>
>> In summary, I think the annotation itself is fine and we are only seeing
>> an issue with its usage after WARN_ENTRY() due to use of
>> asm_volatile_goto. Other uses of unreachable() don't seem to exhibit
>> this problem.
>>
>> As such, I think this patch is appropriate for this series, though I
>> think we should capture some of this information in the changelog.
>>
>> Note also that if and when we start utlizing the annotation, if we
>> classify twui as INSN_BUG, this change will continue to be appropriate.
>>
>
> INSN_TRAP instead of INSN_BUG ?
INSN_BUG, in line with your suggestion here:
http://lkml.kernel.org/r/[email protected]
Peter was of the opinion that INSN_TRAP may not be what we want:
http://lkml.kernel.org/r/YsLSU6idNME/[email protected]
If we classify twui as INSN_BUG, then objtool will know to stop control
flow here without the need for an annotation. Parsing extable will
then show that control flow continues with the label subsequently.
- Naveen
Le 08/08/2022 à 13:48, Sathvika Vasireddy a écrit :
> objtool is throwing *unannotated intra-function call*
> warnings with a few instructions that are marked
> unreachable. Replace unreachable() with __builtin_unreachable()
> to fix these warnings, as the codegen remains same
> with unreachable() and __builtin_unreachable().
>
> Signed-off-by: Sathvika Vasireddy <[email protected]>
> ---
> arch/powerpc/include/asm/bug.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
> index 61a4736355c2..074be1a78c56 100644
> --- a/arch/powerpc/include/asm/bug.h
> +++ b/arch/powerpc/include/asm/bug.h
> @@ -99,7 +99,7 @@
> __label__ __label_warn_on; \
> \
> WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags), __label_warn_on); \
> - unreachable(); \
> + __builtin_unreachable(); \
Should you add barrier_before_unreachable() before
__builtin_unreachable() to avoid stack bombs ?
See commit 173a3efd3edb ("bug.h: work around GCC PR82365 in BUG()")
> \
> __label_warn_on: \
> break; \
Hi Christophe,
On 26/08/22 15:48, Christophe Leroy wrote:
>
> Le 08/08/2022 à 13:48, Sathvika Vasireddy a écrit :
>> objtool is throwing *unannotated intra-function call*
>> warnings with a few instructions that are marked
>> unreachable. Replace unreachable() with __builtin_unreachable()
>> to fix these warnings, as the codegen remains same
>> with unreachable() and __builtin_unreachable().
>>
>> Signed-off-by: Sathvika Vasireddy <[email protected]>
>> ---
>> arch/powerpc/include/asm/bug.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
>> index 61a4736355c2..074be1a78c56 100644
>> --- a/arch/powerpc/include/asm/bug.h
>> +++ b/arch/powerpc/include/asm/bug.h
>> @@ -99,7 +99,7 @@
>> __label__ __label_warn_on; \
>> \
>> WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags), __label_warn_on); \
>> - unreachable(); \
>> + __builtin_unreachable(); \
> Should you add barrier_before_unreachable() before
> __builtin_unreachable() to avoid stack bombs ?
>
> See commit 173a3efd3edb ("bug.h: work around GCC PR82365 in BUG()")
Yes, adding barrier_before_unreachable() before __builtin_unreachable()
works around the build issues reported at
https://lkml.org/lkml/2022/8/25/418.
I'll post a v2 with this change.
Thanks for the suggestion!
- Sathvika