| Message ID | 20231120224912.1421916-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: Enable -Wwrite-strings | expand |
On Mon, 20 Nov 2023, Andrew Cooper wrote: > GCC complains: > > In file included from arch/arm/efi/boot.c:700: > arch/arm/efi/efi-boot.h: In function 'efi_arch_handle_cmdline': > arch/arm/efi/efi-boot.h:482:16: error: assignment discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers] > 482 | name.s = "xen"; > | ^ > > There's no easy option. .rodata is really read-only, so the fact Xen doesn't > crash means these strings aren't written to. > > Lie to the compiler using a union. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Wei Liu <wl@xen.org> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien@xen.org> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > CC: Bertrand Marquis <bertrand.marquis@arm.com> > CC: Michal Orzel <michal.orzel@amd.com> > CC: Roberto Bagnara <roberto.bagnara@bugseng.com> > CC: Nicola Vetrini <nicola.vetrini@bugseng.com> > > I *really* don't like this, but it's the only suggestion given. Hi Andrew, I am trying to understand what is the part you don't like. I understand that union string looks iffy due to being a union with const and non-const. Is that your concern or you also spotted additional problems with this? If that is the only issue, maybe we can come up with a matter in-code comment or commit message. (The TODO is not immediately obvious to me what the issue to be improved is.) > --- > xen/arch/arm/efi/efi-boot.h | 2 +- > xen/arch/x86/efi/efi-boot.h | 3 ++- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h > index 1c3640bb65fd..c26bf18b68b9 100644 > --- a/xen/arch/arm/efi/efi-boot.h > +++ b/xen/arch/arm/efi/efi-boot.h > @@ -479,7 +479,7 @@ static void __init efi_arch_handle_cmdline(CHAR16 *image_name, > w2s(&name); > } > else > - name.s = "xen"; > + name.cs = "xen"; /* TODO, find a better way of doing this. */ > > prop_len = 0; > prop_len += snprintf(buf + prop_len, > diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h > index eebc54180bf7..e2d256e0517b 100644 > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -324,7 +324,8 @@ static void __init efi_arch_handle_cmdline(CHAR16 *image_name, > w2s(&name); > } > else > - name.s = "xen"; > + name.cs = "xen"; /* TODO, find a better way of doing this. */ > + > place_string(&mbi.cmdline, name.s); > > if ( mbi.cmdline ) > -- > 2.30.2 > >
On 20.11.2023 23:49, Andrew Cooper wrote: > GCC complains: > > In file included from arch/arm/efi/boot.c:700: > arch/arm/efi/efi-boot.h: In function 'efi_arch_handle_cmdline': > arch/arm/efi/efi-boot.h:482:16: error: assignment discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers] > 482 | name.s = "xen"; > | ^ > > There's no easy option. .rodata is really read-only, so the fact Xen doesn't > crash means these strings aren't written to. And the consuming sites confirm this being the case. Hence ... > Lie to the compiler using a union. ... to at least slightly limit the lying, how about ... > --- a/xen/arch/arm/efi/efi-boot.h > +++ b/xen/arch/arm/efi/efi-boot.h > @@ -479,7 +479,7 @@ static void __init efi_arch_handle_cmdline(CHAR16 *image_name, > w2s(&name); > } > else > - name.s = "xen"; > + name.cs = "xen"; /* TODO, find a better way of doing this. */ > > prop_len = 0; > prop_len += snprintf(buf + prop_len, ... you also switch to using name.cs down below here and ... > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -324,7 +324,8 @@ static void __init efi_arch_handle_cmdline(CHAR16 *image_name, > w2s(&name); > } > else > - name.s = "xen"; > + name.cs = "xen"; /* TODO, find a better way of doing this. */ > + > place_string(&mbi.cmdline, name.s); ... here? An alternative would be to introduce 'char xen[4] = "xen";' in both cases, and use them instead of plain string literals. Jan
On 21/11/2023 8:40 am, Jan Beulich wrote: > On 20.11.2023 23:49, Andrew Cooper wrote: >> GCC complains: >> >> In file included from arch/arm/efi/boot.c:700: >> arch/arm/efi/efi-boot.h: In function 'efi_arch_handle_cmdline': >> arch/arm/efi/efi-boot.h:482:16: error: assignment discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers] >> 482 | name.s = "xen"; >> | ^ >> >> There's no easy option. .rodata is really read-only, so the fact Xen doesn't >> crash means these strings aren't written to. > And the consuming sites confirm this being the case. Hence ... > >> Lie to the compiler using a union. > ... to at least slightly limit the lying, how about ... > >> --- a/xen/arch/arm/efi/efi-boot.h >> +++ b/xen/arch/arm/efi/efi-boot.h >> @@ -479,7 +479,7 @@ static void __init efi_arch_handle_cmdline(CHAR16 *image_name, >> w2s(&name); >> } >> else >> - name.s = "xen"; >> + name.cs = "xen"; /* TODO, find a better way of doing this. */ >> >> prop_len = 0; >> prop_len += snprintf(buf + prop_len, > ... you also switch to using name.cs down below here and ... > >> --- a/xen/arch/x86/efi/efi-boot.h >> +++ b/xen/arch/x86/efi/efi-boot.h >> @@ -324,7 +324,8 @@ static void __init efi_arch_handle_cmdline(CHAR16 *image_name, >> w2s(&name); >> } >> else >> - name.s = "xen"; >> + name.cs = "xen"; /* TODO, find a better way of doing this. */ >> + >> place_string(&mbi.cmdline, name.s); > ... here? > > An alternative would be to introduce 'char xen[4] = "xen";' in both > cases, and use them instead of plain string literals. They'd have to be static, or dynamically allocated or they'll end up out of scope, wont they? I have to admit I find this logic very hard to follow. Furthermore, I note: mbi.boot_loader_name = (long)"EFI"; which is exactly the kind of pointer which is liable to end up being edited via kextra in the other patch. ~Andrew
On 21/11/2023 6:03 pm, Andrew Cooper wrote: > On 21/11/2023 8:40 am, Jan Beulich wrote: >> On 20.11.2023 23:49, Andrew Cooper wrote: >>> GCC complains: >>> >>> In file included from arch/arm/efi/boot.c:700: >>> arch/arm/efi/efi-boot.h: In function 'efi_arch_handle_cmdline': >>> arch/arm/efi/efi-boot.h:482:16: error: assignment discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers] >>> 482 | name.s = "xen"; >>> | ^ >>> >>> There's no easy option. .rodata is really read-only, so the fact Xen doesn't >>> crash means these strings aren't written to. >> And the consuming sites confirm this being the case. Hence ... >> >>> Lie to the compiler using a union. >> ... to at least slightly limit the lying, how about ... >> >>> --- a/xen/arch/arm/efi/efi-boot.h >>> +++ b/xen/arch/arm/efi/efi-boot.h >>> @@ -479,7 +479,7 @@ static void __init efi_arch_handle_cmdline(CHAR16 *image_name, >>> w2s(&name); >>> } >>> else >>> - name.s = "xen"; >>> + name.cs = "xen"; /* TODO, find a better way of doing this. */ >>> >>> prop_len = 0; >>> prop_len += snprintf(buf + prop_len, >> ... you also switch to using name.cs down below here and ... >> >>> --- a/xen/arch/x86/efi/efi-boot.h >>> +++ b/xen/arch/x86/efi/efi-boot.h >>> @@ -324,7 +324,8 @@ static void __init efi_arch_handle_cmdline(CHAR16 *image_name, >>> w2s(&name); >>> } >>> else >>> - name.s = "xen"; >>> + name.cs = "xen"; /* TODO, find a better way of doing this. */ >>> + >>> place_string(&mbi.cmdline, name.s); >> ... here? >> >> An alternative would be to introduce 'char xen[4] = "xen";' in both >> cases, and use them instead of plain string literals. > They'd have to be static, or dynamically allocated or they'll end up out > of scope, wont they? > > I have to admit I find this logic very hard to follow. > > Furthermore, I note: > > mbi.boot_loader_name = (long)"EFI"; > > which is exactly the kind of pointer which is liable to end up being > edited via kextra in the other patch. Hang on. place_string()'ing here is just to prepend a piece of data we go to other lengths to strip and ignore later in boot. On x86 we can just delete it and make our lives simpler. I hope the same is true on ARM too. ~Andrew
On 21.11.2023 20:13, Andrew Cooper wrote: > On 21/11/2023 6:03 pm, Andrew Cooper wrote: >> On 21/11/2023 8:40 am, Jan Beulich wrote: >>> On 20.11.2023 23:49, Andrew Cooper wrote: >>>> GCC complains: >>>> >>>> In file included from arch/arm/efi/boot.c:700: >>>> arch/arm/efi/efi-boot.h: In function 'efi_arch_handle_cmdline': >>>> arch/arm/efi/efi-boot.h:482:16: error: assignment discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers] >>>> 482 | name.s = "xen"; >>>> | ^ >>>> >>>> There's no easy option. .rodata is really read-only, so the fact Xen doesn't >>>> crash means these strings aren't written to. >>> And the consuming sites confirm this being the case. Hence ... >>> >>>> Lie to the compiler using a union. >>> ... to at least slightly limit the lying, how about ... >>> >>>> --- a/xen/arch/arm/efi/efi-boot.h >>>> +++ b/xen/arch/arm/efi/efi-boot.h >>>> @@ -479,7 +479,7 @@ static void __init efi_arch_handle_cmdline(CHAR16 *image_name, >>>> w2s(&name); >>>> } >>>> else >>>> - name.s = "xen"; >>>> + name.cs = "xen"; /* TODO, find a better way of doing this. */ >>>> >>>> prop_len = 0; >>>> prop_len += snprintf(buf + prop_len, >>> ... you also switch to using name.cs down below here and ... >>> >>>> --- a/xen/arch/x86/efi/efi-boot.h >>>> +++ b/xen/arch/x86/efi/efi-boot.h >>>> @@ -324,7 +324,8 @@ static void __init efi_arch_handle_cmdline(CHAR16 *image_name, >>>> w2s(&name); >>>> } >>>> else >>>> - name.s = "xen"; >>>> + name.cs = "xen"; /* TODO, find a better way of doing this. */ >>>> + >>>> place_string(&mbi.cmdline, name.s); >>> ... here? >>> >>> An alternative would be to introduce 'char xen[4] = "xen";' in both >>> cases, and use them instead of plain string literals. >> They'd have to be static, or dynamically allocated or they'll end up out >> of scope, wont they? No, place_string() copies into the target area. The input string then isn't further used. >> I have to admit I find this logic very hard to follow. >> >> Furthermore, I note: >> >> mbi.boot_loader_name = (long)"EFI"; >> >> which is exactly the kind of pointer which is liable to end up being >> edited via kextra in the other patch. > > Hang on. place_string()'ing here is just to prepend a piece of data we > go to other lengths to strip and ignore later in boot. > > On x86 we can just delete it and make our lives simpler. I hope the > same is true on ARM too. Well, yes, the prepending is just to allow uniform handling later on. Surely this can be done differently. Again - I'll go look at v2. Jan
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h index 1c3640bb65fd..c26bf18b68b9 100644 --- a/xen/arch/arm/efi/efi-boot.h +++ b/xen/arch/arm/efi/efi-boot.h @@ -479,7 +479,7 @@ static void __init efi_arch_handle_cmdline(CHAR16 *image_name, w2s(&name); } else - name.s = "xen"; + name.cs = "xen"; /* TODO, find a better way of doing this. */ prop_len = 0; prop_len += snprintf(buf + prop_len, diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index eebc54180bf7..e2d256e0517b 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -324,7 +324,8 @@ static void __init efi_arch_handle_cmdline(CHAR16 *image_name, w2s(&name); } else - name.s = "xen"; + name.cs = "xen"; /* TODO, find a better way of doing this. */ + place_string(&mbi.cmdline, name.s); if ( mbi.cmdline )
GCC complains: In file included from arch/arm/efi/boot.c:700: arch/arm/efi/efi-boot.h: In function 'efi_arch_handle_cmdline': arch/arm/efi/efi-boot.h:482:16: error: assignment discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers] 482 | name.s = "xen"; | ^ There's no easy option. .rodata is really read-only, so the fact Xen doesn't crash means these strings aren't written to. Lie to the compiler using a union. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Bertrand Marquis <bertrand.marquis@arm.com> CC: Michal Orzel <michal.orzel@amd.com> CC: Roberto Bagnara <roberto.bagnara@bugseng.com> CC: Nicola Vetrini <nicola.vetrini@bugseng.com> I *really* don't like this, but it's the only suggestion given. --- xen/arch/arm/efi/efi-boot.h | 2 +- xen/arch/x86/efi/efi-boot.h | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-)