| Message ID | 20210405155713.29754-1-julien@xen.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Use const whether we point to literal strings (take 1) | expand |
On Mon, Apr 05, 2021 at 04:56:59PM +0100, Julien Grall wrote: > I am not aware of code trying to modify literal strings in Xen. > However, there is a frequent use of non-const char * to point to > literal strings. Given the size of the codebase, there is a risk > to involuntarily introduce code that will modify literal strings. > > Therefore it would be better to enforce using const when pointing > to such strings. Both GCC and Clang provide an option to warn > for such case (see -Wwrite-strings) and therefore could be used > by Xen. > > This series doesn't yet make use of -Wwrite-strings because > the tree is not fully converted. Instead, it contains some easy > and likely non-controversial use const in the code. > > The major blockers to enable -Wwrite-strings are the following: > - xen/common/efi: union string is used in both const and > non-const situation. It doesn't feel right to specific one member > const and the other non-const. > - libxl: the major block is the flexarray framework as we would use > it with string (now const char*). I thought it would be possible to > make the interface const, but it looks like there are a couple of > places where we need to modify the content (such as in > libxl_json.c). > > Ideally, I would like to have -Wwrite-strings unconditionally used > tree-wide. But, some of the area may required some heavy refactoring. > > One solution would be to enable it tree-wide but turned it off at a > directroy/file level. > > Any opinions? I think doing such is a Good Idea(tm). Adding consts adds opportunities for greater optimization. Both by compilers which can avoid extra copies, and developers who then know they don't need to generate extra copies to sacrific them to an API. In particular the consts also function as documentation. So you're certainly not the only person who thinks additional consts would be a good thing: https://lists.xenproject.org/archives/html/xen-devel/2021-01/msg00132.html Alas merely getting the two const patches into the latest release apparently wasn't even worthy of response: https://lists.xenproject.org/archives/html/xen-devel/2021-02/msg01040.html I agree this should be done, though I'm aware many developers hate bothering with constants.
On 05.04.2021 17:56, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > Hi all, > > By default, both Clang and GCC will happily compile C code where > non-const char * point to literal strings. This means the following > code will be accepted: > > char *str = "test"; > > str[0] = 'a'; > > Literal strings will reside in rodata, so they are not modifiable. > This will result to an permission fault at runtime if the permissions > are enforced in the page-tables (this is the case in Xen). > > I am not aware of code trying to modify literal strings in Xen. > However, there is a frequent use of non-const char * to point to > literal strings. Given the size of the codebase, there is a risk > to involuntarily introduce code that will modify literal strings. > > Therefore it would be better to enforce using const when pointing > to such strings. Both GCC and Clang provide an option to warn > for such case (see -Wwrite-strings) and therefore could be used > by Xen. > > This series doesn't yet make use of -Wwrite-strings because > the tree is not fully converted. Instead, it contains some easy > and likely non-controversial use const in the code. > > The major blockers to enable -Wwrite-strings are the following: > - xen/common/efi: union string is used in both const and > non-const situation. It doesn't feel right to specific one member > const and the other non-const. I'd be happy to see a suggestion of how to avoid this in a not overly intrusive way. > - libxl: the major block is the flexarray framework as we would use > it with string (now const char*). I thought it would be possible to > make the interface const, but it looks like there are a couple of > places where we need to modify the content (such as in > libxl_json.c). > > Ideally, I would like to have -Wwrite-strings unconditionally used > tree-wide. But, some of the area may required some heavy refactoring. > > One solution would be to enable it tree-wide but turned it off at a > directroy/file level. At least as a transient approach I think this would make sense. EFI in particular has other reasons already to specify a custom option (-fshort-wchar). Jan
Hi Elliott, Thanks for the input! On 05/04/2021 18:01, Elliott Mitchell wrote: > On Mon, Apr 05, 2021 at 04:56:59PM +0100, Julien Grall wrote: >> I am not aware of code trying to modify literal strings in Xen. >> However, there is a frequent use of non-const char * to point to >> literal strings. Given the size of the codebase, there is a risk >> to involuntarily introduce code that will modify literal strings. >> >> Therefore it would be better to enforce using const when pointing >> to such strings. Both GCC and Clang provide an option to warn >> for such case (see -Wwrite-strings) and therefore could be used >> by Xen. >> >> This series doesn't yet make use of -Wwrite-strings because >> the tree is not fully converted. Instead, it contains some easy >> and likely non-controversial use const in the code. >> >> The major blockers to enable -Wwrite-strings are the following: >> - xen/common/efi: union string is used in both const and >> non-const situation. It doesn't feel right to specific one member >> const and the other non-const. >> - libxl: the major block is the flexarray framework as we would use >> it with string (now const char*). I thought it would be possible to >> make the interface const, but it looks like there are a couple of >> places where we need to modify the content (such as in >> libxl_json.c). >> >> Ideally, I would like to have -Wwrite-strings unconditionally used >> tree-wide. But, some of the area may required some heavy refactoring. >> >> One solution would be to enable it tree-wide but turned it off at a >> directroy/file level. >> >> Any opinions? > > I think doing such is a Good Idea(tm). Adding consts adds opportunities > for greater optimization. Both by compilers which can avoid extra > copies, and developers who then know they don't need to generate extra > copies to sacrific them to an API. In particular the consts also > function as documentation. > > So you're certainly not the only person who thinks additional consts > would be a good thing: > > https://lists.xenproject.org/archives/html/xen-devel/2021-01/msg00132.html > > > Alas merely getting the two const patches into the latest release > apparently wasn't even worthy of response: > > https://lists.xenproject.org/archives/html/xen-devel/2021-02/msg01040.html Sory to hear you had no response. Would you be able to give a nudge to the maintainers? > > > I agree this should be done, though I'm aware many developers hate > bothering with constants. > > Cheers,
Hi, On 05/04/2021 16:56, Julien Grall wrote: > Julien Grall (14): > xen: Constify the second parameter of rangeset_new() > xen/sched: Constify name and opt_name in struct scheduler > xen/x86: shadow: The return type of sh_audit_flags() should be const > tools/firmware: hvmloader: Use const in __bug() and __assert_failed() > tools/kdd: Use const whenever we point to literal strings > tools/xentrace: Use const whenever we point to literal strings I have merged the above patches. The rest still needs some reviews or respin (for patch #4). Cheers,
Hi, Ian, Wei, Anthony, can I get some feedbacks on the tools side? Cheers, On 05/04/2021 16:56, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > Hi all, > > By default, both Clang and GCC will happily compile C code where > non-const char * point to literal strings. This means the following > code will be accepted: > > char *str = "test"; > > str[0] = 'a'; > > Literal strings will reside in rodata, so they are not modifiable. > This will result to an permission fault at runtime if the permissions > are enforced in the page-tables (this is the case in Xen). > > I am not aware of code trying to modify literal strings in Xen. > However, there is a frequent use of non-const char * to point to > literal strings. Given the size of the codebase, there is a risk > to involuntarily introduce code that will modify literal strings. > > Therefore it would be better to enforce using const when pointing > to such strings. Both GCC and Clang provide an option to warn > for such case (see -Wwrite-strings) and therefore could be used > by Xen. > > This series doesn't yet make use of -Wwrite-strings because > the tree is not fully converted. Instead, it contains some easy > and likely non-controversial use const in the code. > > The major blockers to enable -Wwrite-strings are the following: > - xen/common/efi: union string is used in both const and > non-const situation. It doesn't feel right to specific one member > const and the other non-const. > - libxl: the major block is the flexarray framework as we would use > it with string (now const char*). I thought it would be possible to > make the interface const, but it looks like there are a couple of > places where we need to modify the content (such as in > libxl_json.c). > > Ideally, I would like to have -Wwrite-strings unconditionally used > tree-wide. But, some of the area may required some heavy refactoring. > > One solution would be to enable it tree-wide but turned it off at a > directroy/file level. > > Any opinions? > > Cheers, > > Julien Grall (14): > xen: Constify the second parameter of rangeset_new() > xen/sched: Constify name and opt_name in struct scheduler > xen/x86: shadow: The return type of sh_audit_flags() should be const > xen/char: console: Use const whenever we point to literal strings > tools/libs: guest: Use const whenever we point to literal strings > tools/libs: stat: Use const whenever we point to literal strings > tools/xl: Use const whenever we point to literal strings > tools/firmware: hvmloader: Use const in __bug() and __assert_failed() > tools/console: Use const whenever we point to literal strings > tools/kdd: Use const whenever we point to literal strings > tools/misc: Use const whenever we point to literal strings > tools/top: The string parameter in set_prompt() and set_delay() should > be const > tools/xenmon: xenbaked: Mark const the field text in stat_map_t > tools/xentrace: Use const whenever we point to literal strings > > tools/console/client/main.c | 4 +- > tools/console/daemon/io.c | 10 ++-- > tools/debugger/kdd/kdd.c | 10 ++-- > tools/firmware/hvmloader/util.c | 4 +- > tools/firmware/hvmloader/util.h | 4 +- > tools/include/xenguest.h | 10 ++-- > tools/libs/guest/xg_dom_core.c | 8 ++-- > tools/libs/guest/xg_dom_elfloader.c | 4 +- > tools/libs/guest/xg_dom_hvmloader.c | 2 +- > tools/libs/guest/xg_dom_x86.c | 9 ++-- > tools/libs/guest/xg_private.h | 2 +- > tools/libs/stat/xenstat_linux.c | 4 +- > tools/libs/stat/xenstat_qmp.c | 12 ++--- > tools/misc/xen-detect.c | 2 +- > tools/misc/xenhypfs.c | 6 +-- > tools/xenmon/xenbaked.c | 2 +- > tools/xentop/xentop.c | 12 ++--- > tools/xentrace/xenalyze.c | 71 +++++++++++++++-------------- > tools/xentrace/xenctx.c | 4 +- > tools/xl/xl.h | 8 ++-- > tools/xl/xl_console.c | 2 +- > tools/xl/xl_utils.c | 4 +- > tools/xl/xl_utils.h | 4 +- > xen/arch/x86/mm/shadow/multi.c | 12 ++--- > xen/common/rangeset.c | 2 +- > xen/common/sched/private.h | 4 +- > xen/drivers/char/console.c | 4 +- > xen/include/xen/rangeset.h | 2 +- > 28 files changed, 113 insertions(+), 109 deletions(-) >
On Mon, May 10, 2021 at 06:49:01PM +0100, Julien Grall wrote: > Hi, > > Ian, Wei, Anthony, can I get some feedbacks on the tools side? I think this is moving to the right direction so Acked-by: Wei Liu <wl@xen.org>
Hi Wei, On 17/05/2021 19:41, Wei Liu wrote: > On Mon, May 10, 2021 at 06:49:01PM +0100, Julien Grall wrote: >> Hi, >> >> Ian, Wei, Anthony, can I get some feedbacks on the tools side? > > I think this is moving to the right direction so > > Acked-by: Wei Liu <wl@xen.org> Thanks! I had committed most of the tools code but one patch. I have kept your acked-by on the patch and will wait Anthony to do a full review. Cheers,
From: Julien Grall <jgrall@amazon.com> Hi all, By default, both Clang and GCC will happily compile C code where non-const char * point to literal strings. This means the following code will be accepted: char *str = "test"; str[0] = 'a'; Literal strings will reside in rodata, so they are not modifiable. This will result to an permission fault at runtime if the permissions are enforced in the page-tables (this is the case in Xen). I am not aware of code trying to modify literal strings in Xen. However, there is a frequent use of non-const char * to point to literal strings. Given the size of the codebase, there is a risk to involuntarily introduce code that will modify literal strings. Therefore it would be better to enforce using const when pointing to such strings. Both GCC and Clang provide an option to warn for such case (see -Wwrite-strings) and therefore could be used by Xen. This series doesn't yet make use of -Wwrite-strings because the tree is not fully converted. Instead, it contains some easy and likely non-controversial use const in the code. The major blockers to enable -Wwrite-strings are the following: - xen/common/efi: union string is used in both const and non-const situation. It doesn't feel right to specific one member const and the other non-const. - libxl: the major block is the flexarray framework as we would use it with string (now const char*). I thought it would be possible to make the interface const, but it looks like there are a couple of places where we need to modify the content (such as in libxl_json.c). Ideally, I would like to have -Wwrite-strings unconditionally used tree-wide. But, some of the area may required some heavy refactoring. One solution would be to enable it tree-wide but turned it off at a directroy/file level. Any opinions? Cheers, Julien Grall (14): xen: Constify the second parameter of rangeset_new() xen/sched: Constify name and opt_name in struct scheduler xen/x86: shadow: The return type of sh_audit_flags() should be const xen/char: console: Use const whenever we point to literal strings tools/libs: guest: Use const whenever we point to literal strings tools/libs: stat: Use const whenever we point to literal strings tools/xl: Use const whenever we point to literal strings tools/firmware: hvmloader: Use const in __bug() and __assert_failed() tools/console: Use const whenever we point to literal strings tools/kdd: Use const whenever we point to literal strings tools/misc: Use const whenever we point to literal strings tools/top: The string parameter in set_prompt() and set_delay() should be const tools/xenmon: xenbaked: Mark const the field text in stat_map_t tools/xentrace: Use const whenever we point to literal strings tools/console/client/main.c | 4 +- tools/console/daemon/io.c | 10 ++-- tools/debugger/kdd/kdd.c | 10 ++-- tools/firmware/hvmloader/util.c | 4 +- tools/firmware/hvmloader/util.h | 4 +- tools/include/xenguest.h | 10 ++-- tools/libs/guest/xg_dom_core.c | 8 ++-- tools/libs/guest/xg_dom_elfloader.c | 4 +- tools/libs/guest/xg_dom_hvmloader.c | 2 +- tools/libs/guest/xg_dom_x86.c | 9 ++-- tools/libs/guest/xg_private.h | 2 +- tools/libs/stat/xenstat_linux.c | 4 +- tools/libs/stat/xenstat_qmp.c | 12 ++--- tools/misc/xen-detect.c | 2 +- tools/misc/xenhypfs.c | 6 +-- tools/xenmon/xenbaked.c | 2 +- tools/xentop/xentop.c | 12 ++--- tools/xentrace/xenalyze.c | 71 +++++++++++++++-------------- tools/xentrace/xenctx.c | 4 +- tools/xl/xl.h | 8 ++-- tools/xl/xl_console.c | 2 +- tools/xl/xl_utils.c | 4 +- tools/xl/xl_utils.h | 4 +- xen/arch/x86/mm/shadow/multi.c | 12 ++--- xen/common/rangeset.c | 2 +- xen/common/sched/private.h | 4 +- xen/drivers/char/console.c | 4 +- xen/include/xen/rangeset.h | 2 +- 28 files changed, 113 insertions(+), 109 deletions(-)