Currently some parts of the codebase will test for CONFIG_COMPAT before
testing is_compat_task().
is_compat_task() is a inlined function only present on CONFIG_COMPAT.
On the other hand, for !CONFIG_COMPAT, we have in linux/compat.h:
#define is_compat_task() (0)
Since we have this define available in every usage of is_compat_task() for
!CONFIG_COMPAT, it's unnecessary to keep the ifdefs, since the compiler is
smart enough to optimize-out those snippets on CONFIG_COMPAT=n
Signed-off-by: Leonardo Bras <[email protected]>
---
Changes since RFCv1:
- Removed unnecessary new inlined is_compat_task() for arm64
- Adjusted commit text and title
Link: https://lore.kernel.org/all/[email protected]/
arch/arm64/kernel/ptrace.c | 6 ++----
arch/arm64/kernel/syscall.c | 5 +----
2 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 20d7ef82de90a..9f8781f1fdfda 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -173,7 +173,6 @@ static void ptrace_hbptriggered(struct perf_event *bp,
struct arch_hw_breakpoint *bkpt = counter_arch_bp(bp);
const char *desc = "Hardware breakpoint trap (ptrace)";
-#ifdef CONFIG_COMPAT
if (is_compat_task()) {
int si_errno = 0;
int i;
@@ -195,7 +194,7 @@ static void ptrace_hbptriggered(struct perf_event *bp,
desc);
return;
}
-#endif
+
arm64_force_sig_fault(SIGTRAP, TRAP_HWBKPT, bkpt->trigger, desc);
}
@@ -2112,7 +2111,6 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
const struct user_regset_view *task_user_regset_view(struct task_struct *task)
{
-#ifdef CONFIG_COMPAT
/*
* Core dumping of 32-bit tasks or compat ptrace requests must use the
* user_aarch32_view compatible with arm32. Native ptrace requests on
@@ -2123,7 +2121,7 @@ const struct user_regset_view *task_user_regset_view(struct task_struct *task)
return &user_aarch32_view;
else if (is_compat_thread(task_thread_info(task)))
return &user_aarch32_ptrace_view;
-#endif
+
return &user_aarch64_view;
}
diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
index 9a70d9746b661..ad198262b9817 100644
--- a/arch/arm64/kernel/syscall.c
+++ b/arch/arm64/kernel/syscall.c
@@ -20,14 +20,11 @@ long sys_ni_syscall(void);
static long do_ni_syscall(struct pt_regs *regs, int scno)
{
-#ifdef CONFIG_COMPAT
- long ret;
if (is_compat_task()) {
- ret = compat_arm_syscall(regs, scno);
+ long ret = compat_arm_syscall(regs, scno);
if (ret != -ENOSYS)
return ret;
}
-#endif
return sys_ni_syscall();
}
--
2.43.0
On Fri, Jan 5, 2024, at 05:15, Leonardo Bras wrote:
> Currently some parts of the codebase will test for CONFIG_COMPAT before
> testing is_compat_task().
>
> is_compat_task() is a inlined function only present on CONFIG_COMPAT.
> On the other hand, for !CONFIG_COMPAT, we have in linux/compat.h:
>
> #define is_compat_task() (0)
>
> Since we have this define available in every usage of is_compat_task() for
> !CONFIG_COMPAT, it's unnecessary to keep the ifdefs, since the compiler is
> smart enough to optimize-out those snippets on CONFIG_COMPAT=n
>
> Signed-off-by: Leonardo Bras <[email protected]>
Reviewed-by: Arnd Bergmann <[email protected]>
On Fri, Jan 05, 2024 at 01:15:00AM -0300, Leonardo Bras wrote:
> Currently some parts of the codebase will test for CONFIG_COMPAT before
> testing is_compat_task().
>
> is_compat_task() is a inlined function only present on CONFIG_COMPAT.
> On the other hand, for !CONFIG_COMPAT, we have in linux/compat.h:
>
> #define is_compat_task() (0)
>
> Since we have this define available in every usage of is_compat_task() for
> !CONFIG_COMPAT, it's unnecessary to keep the ifdefs, since the compiler is
> smart enough to optimize-out those snippets on CONFIG_COMPAT=n
>
> Signed-off-by: Leonardo Bras <[email protected]>
I tried this atop the arm64 for-next/core branch, using GCC 13.2.0; building
defconfig + CONFIG_COMPAT=n results in build errors:
[mark@lakrids:~/src/linux]% usekorg 13.2.0 make ARCH=arm64 CROSS_COMPILE=aarch64-linux- Image
CALL scripts/checksyscalls.sh
CC arch/arm64/kernel/ptrace.o
arch/arm64/kernel/ptrace.c: In function 'task_user_regset_view':
arch/arm64/kernel/ptrace.c:2121:25: error: 'user_aarch32_view' undeclared (first use in this function); did you mean 'user_aarch64_view'?
2121 | return &user_aarch32_view;
| ^~~~~~~~~~~~~~~~~
| user_aarch64_view
arch/arm64/kernel/ptrace.c:2121:25: note: each undeclared identifier is reported only once for each function it appears in
arch/arm64/kernel/ptrace.c:2123:25: error: 'user_aarch32_ptrace_view' undeclared (first use in this function)
2123 | return &user_aarch32_ptrace_view;
| ^~~~~~~~~~~~~~~~~~~~~~~~
make[4]: *** [scripts/Makefile.build:243: arch/arm64/kernel/ptrace.o] Error 1
make[3]: *** [scripts/Makefile.build:480: arch/arm64/kernel] Error 2
make[2]: *** [scripts/Makefile.build:480: arch/arm64] Error 2
make[1]: *** [/home/mark/src/linux/Makefile:1911: .] Error 2
make: *** [Makefile:234: __sub-make] Error 2
... and looking at the code, user_aarch32_view and user_aarch32_ptrace_view are
both defined under ifdeffery for CONFIG_COMPAT, so that's obviously not going
to work...
That aside, removing ifdeffery is generally nice, so could you please try
building with CONFIG_COMPAT=n and see where you get to?
Thanks,
Mark.
> ---
> Changes since RFCv1:
> - Removed unnecessary new inlined is_compat_task() for arm64
> - Adjusted commit text and title
> Link: https://lore.kernel.org/all/[email protected]/
>
> arch/arm64/kernel/ptrace.c | 6 ++----
> arch/arm64/kernel/syscall.c | 5 +----
> 2 files changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index 20d7ef82de90a..9f8781f1fdfda 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -173,7 +173,6 @@ static void ptrace_hbptriggered(struct perf_event *bp,
> struct arch_hw_breakpoint *bkpt = counter_arch_bp(bp);
> const char *desc = "Hardware breakpoint trap (ptrace)";
>
> -#ifdef CONFIG_COMPAT
> if (is_compat_task()) {
> int si_errno = 0;
> int i;
> @@ -195,7 +194,7 @@ static void ptrace_hbptriggered(struct perf_event *bp,
> desc);
> return;
> }
> -#endif
> +
> arm64_force_sig_fault(SIGTRAP, TRAP_HWBKPT, bkpt->trigger, desc);
> }
>
> @@ -2112,7 +2111,6 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
>
> const struct user_regset_view *task_user_regset_view(struct task_struct *task)
> {
> -#ifdef CONFIG_COMPAT
> /*
> * Core dumping of 32-bit tasks or compat ptrace requests must use the
> * user_aarch32_view compatible with arm32. Native ptrace requests on
> @@ -2123,7 +2121,7 @@ const struct user_regset_view *task_user_regset_view(struct task_struct *task)
> return &user_aarch32_view;
> else if (is_compat_thread(task_thread_info(task)))
> return &user_aarch32_ptrace_view;
> -#endif
> +
> return &user_aarch64_view;
> }
>
> diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
> index 9a70d9746b661..ad198262b9817 100644
> --- a/arch/arm64/kernel/syscall.c
> +++ b/arch/arm64/kernel/syscall.c
> @@ -20,14 +20,11 @@ long sys_ni_syscall(void);
>
> static long do_ni_syscall(struct pt_regs *regs, int scno)
> {
> -#ifdef CONFIG_COMPAT
> - long ret;
> if (is_compat_task()) {
> - ret = compat_arm_syscall(regs, scno);
> + long ret = compat_arm_syscall(regs, scno);
> if (ret != -ENOSYS)
> return ret;
> }
> -#endif
>
> return sys_ni_syscall();
> }
> --
> 2.43.0
>
On Fri, Jan 5, 2024, at 14:14, Mark Rutland wrote:
> On Fri, Jan 05, 2024 at 01:15:00AM -0300, Leonardo Bras wrote:
> arch/arm64/kernel/ptrace.c:2121:25: note: each undeclared identifier is
> reported only once for each function it appears in
> arch/arm64/kernel/ptrace.c:2123:25: error: 'user_aarch32_ptrace_view'
> undeclared (first use in this function)
> 2123 | return &user_aarch32_ptrace_view;
> | ^~~~~~~~~~~~~~~~~~~~~~~~
> make[4]: *** [scripts/Makefile.build:243: arch/arm64/kernel/ptrace.o]
> Error 1
> make[3]: *** [scripts/Makefile.build:480: arch/arm64/kernel] Error 2
> make[2]: *** [scripts/Makefile.build:480: arch/arm64] Error 2
> make[1]: *** [/home/mark/src/linux/Makefile:1911: .] Error 2
> make: *** [Makefile:234: __sub-make] Error 2
>
> ... and looking at the code, user_aarch32_view and user_aarch32_ptrace_view are
> both defined under ifdeffery for CONFIG_COMPAT, so that's obviously not going
> to work...
I suspect it's enough to remove all of the other
"#ifdef CONFIG_COMPAT" checks in this file and rely on
dead code elimination to remove the rest, but there might
be additional problems if some extern declarations are
hidden in an #ifdef as well.
Arnd
Hi Leonardo,
kernel test robot noticed the following build errors:
[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on linus/master v6.7-rc8 next-20240105]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Leonardo-Bras/arm64-remove-unnecessary-ifdefs-around-is_compat_task/20240105-121957
base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
patch link: https://lore.kernel.org/r/20240105041458.126602-3-leobras%40redhat.com
patch subject: [PATCH v1 1/1] arm64: remove unnecessary ifdefs around is_compat_task()
config: arm64-randconfig-001-20240105 (https://download.01.org/0day-ci/archive/20240106/[email protected]/config)
compiler: clang version 18.0.0git (https://github.com/llvm/llvm-project 7e186d366d6c7def0543acc255931f617e76dff0)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240106/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> arch/arm64/kernel/ptrace.c:2121:11: error: use of undeclared identifier 'user_aarch32_view'; did you mean 'user_aarch64_view'?
2121 | return &user_aarch32_view;
| ^~~~~~~~~~~~~~~~~
| user_aarch64_view
arch/arm64/kernel/ptrace.c:1591:38: note: 'user_aarch64_view' declared here
1591 | static const struct user_regset_view user_aarch64_view = {
| ^
>> arch/arm64/kernel/ptrace.c:2123:11: error: use of undeclared identifier 'user_aarch32_ptrace_view'
2123 | return &user_aarch32_ptrace_view;
| ^
2 errors generated.
vim +2121 arch/arm64/kernel/ptrace.c
478fcb2cdb2351 Will Deacon 2012-03-05 2111
478fcb2cdb2351 Will Deacon 2012-03-05 2112 const struct user_regset_view *task_user_regset_view(struct task_struct *task)
478fcb2cdb2351 Will Deacon 2012-03-05 2113 {
5d220ff9420f8b Catalin Marinas 2015-07-14 2114 /*
5d220ff9420f8b Catalin Marinas 2015-07-14 2115 * Core dumping of 32-bit tasks or compat ptrace requests must use the
5d220ff9420f8b Catalin Marinas 2015-07-14 2116 * user_aarch32_view compatible with arm32. Native ptrace requests on
5d220ff9420f8b Catalin Marinas 2015-07-14 2117 * 32-bit children use an extended user_aarch32_ptrace_view to allow
5d220ff9420f8b Catalin Marinas 2015-07-14 2118 * access to the TLS register.
5d220ff9420f8b Catalin Marinas 2015-07-14 2119 */
5d220ff9420f8b Catalin Marinas 2015-07-14 2120 if (is_compat_task())
478fcb2cdb2351 Will Deacon 2012-03-05 @2121 return &user_aarch32_view;
5d220ff9420f8b Catalin Marinas 2015-07-14 2122 else if (is_compat_thread(task_thread_info(task)))
5d220ff9420f8b Catalin Marinas 2015-07-14 @2123 return &user_aarch32_ptrace_view;
e802ab35101cdf Leonardo Bras 2024-01-05 2124
478fcb2cdb2351 Will Deacon 2012-03-05 2125 return &user_aarch64_view;
478fcb2cdb2351 Will Deacon 2012-03-05 2126 }
478fcb2cdb2351 Will Deacon 2012-03-05 2127
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Leonardo,
kernel test robot noticed the following build errors:
[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on linus/master v6.7-rc8 next-20240105]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Leonardo-Bras/arm64-remove-unnecessary-ifdefs-around-is_compat_task/20240105-121957
base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
patch link: https://lore.kernel.org/r/20240105041458.126602-3-leobras%40redhat.com
patch subject: [PATCH v1 1/1] arm64: remove unnecessary ifdefs around is_compat_task()
config: arm64-allnoconfig (https://download.01.org/0day-ci/archive/20240106/[email protected]/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240106/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
arch/arm64/kernel/ptrace.c: In function 'task_user_regset_view':
>> arch/arm64/kernel/ptrace.c:2121:25: error: 'user_aarch32_view' undeclared (first use in this function); did you mean 'user_aarch64_view'?
2121 | return &user_aarch32_view;
| ^~~~~~~~~~~~~~~~~
| user_aarch64_view
arch/arm64/kernel/ptrace.c:2121:25: note: each undeclared identifier is reported only once for each function it appears in
>> arch/arm64/kernel/ptrace.c:2123:25: error: 'user_aarch32_ptrace_view' undeclared (first use in this function)
2123 | return &user_aarch32_ptrace_view;
| ^~~~~~~~~~~~~~~~~~~~~~~~
vim +2121 arch/arm64/kernel/ptrace.c
478fcb2cdb2351 Will Deacon 2012-03-05 2111
478fcb2cdb2351 Will Deacon 2012-03-05 2112 const struct user_regset_view *task_user_regset_view(struct task_struct *task)
478fcb2cdb2351 Will Deacon 2012-03-05 2113 {
5d220ff9420f8b Catalin Marinas 2015-07-14 2114 /*
5d220ff9420f8b Catalin Marinas 2015-07-14 2115 * Core dumping of 32-bit tasks or compat ptrace requests must use the
5d220ff9420f8b Catalin Marinas 2015-07-14 2116 * user_aarch32_view compatible with arm32. Native ptrace requests on
5d220ff9420f8b Catalin Marinas 2015-07-14 2117 * 32-bit children use an extended user_aarch32_ptrace_view to allow
5d220ff9420f8b Catalin Marinas 2015-07-14 2118 * access to the TLS register.
5d220ff9420f8b Catalin Marinas 2015-07-14 2119 */
5d220ff9420f8b Catalin Marinas 2015-07-14 2120 if (is_compat_task())
478fcb2cdb2351 Will Deacon 2012-03-05 @2121 return &user_aarch32_view;
5d220ff9420f8b Catalin Marinas 2015-07-14 2122 else if (is_compat_thread(task_thread_info(task)))
5d220ff9420f8b Catalin Marinas 2015-07-14 @2123 return &user_aarch32_ptrace_view;
e802ab35101cdf Leonardo Bras 2024-01-05 2124
478fcb2cdb2351 Will Deacon 2012-03-05 2125 return &user_aarch64_view;
478fcb2cdb2351 Will Deacon 2012-03-05 2126 }
478fcb2cdb2351 Will Deacon 2012-03-05 2127
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Fri, Jan 05, 2024 at 03:38:05PM +0100, Arnd Bergmann wrote:
> On Fri, Jan 5, 2024, at 14:14, Mark Rutland wrote:
> > On Fri, Jan 05, 2024 at 01:15:00AM -0300, Leonardo Bras wrote:
> > arch/arm64/kernel/ptrace.c:2121:25: note: each undeclared identifier is
> > reported only once for each function it appears in
> > arch/arm64/kernel/ptrace.c:2123:25: error: 'user_aarch32_ptrace_view'
> > undeclared (first use in this function)
> > 2123 | return &user_aarch32_ptrace_view;
> > | ^~~~~~~~~~~~~~~~~~~~~~~~
> > make[4]: *** [scripts/Makefile.build:243: arch/arm64/kernel/ptrace.o]
> > Error 1
> > make[3]: *** [scripts/Makefile.build:480: arch/arm64/kernel] Error 2
> > make[2]: *** [scripts/Makefile.build:480: arch/arm64] Error 2
> > make[1]: *** [/home/mark/src/linux/Makefile:1911: .] Error 2
> > make: *** [Makefile:234: __sub-make] Error 2
> >
> > ... and looking at the code, user_aarch32_view and user_aarch32_ptrace_view are
> > both defined under ifdeffery for CONFIG_COMPAT, so that's obviously not going
> > to work...
Thanks for noticing, Mark!
>
> I suspect it's enough to remove all of the other
> "#ifdef CONFIG_COMPAT" checks in this file and rely on
> dead code elimination to remove the rest, but there might
> be additional problems if some extern declarations are
> hidden in an #ifdef as well.
>
> Arnd
Sure, I sill send a v2 soon.
Thanks!
Leo
>
On Mon, Jan 08, 2024 at 12:07:48PM -0300, Leonardo Bras wrote:
> On Fri, Jan 05, 2024 at 03:38:05PM +0100, Arnd Bergmann wrote:
> > On Fri, Jan 5, 2024, at 14:14, Mark Rutland wrote:
> > > On Fri, Jan 05, 2024 at 01:15:00AM -0300, Leonardo Bras wrote:
> > > arch/arm64/kernel/ptrace.c:2121:25: note: each undeclared identifier is
> > > reported only once for each function it appears in
> > > arch/arm64/kernel/ptrace.c:2123:25: error: 'user_aarch32_ptrace_view'
> > > undeclared (first use in this function)
> > > 2123 | return &user_aarch32_ptrace_view;
> > > | ^~~~~~~~~~~~~~~~~~~~~~~~
> > > make[4]: *** [scripts/Makefile.build:243: arch/arm64/kernel/ptrace.o]
> > > Error 1
> > > make[3]: *** [scripts/Makefile.build:480: arch/arm64/kernel] Error 2
> > > make[2]: *** [scripts/Makefile.build:480: arch/arm64] Error 2
> > > make[1]: *** [/home/mark/src/linux/Makefile:1911: .] Error 2
> > > make: *** [Makefile:234: __sub-make] Error 2
> > >
> > > ... and looking at the code, user_aarch32_view and user_aarch32_ptrace_view are
> > > both defined under ifdeffery for CONFIG_COMPAT, so that's obviously not going
> > > to work...
>
> Thanks for noticing, Mark!
>
> >
> > I suspect it's enough to remove all of the other
> > "#ifdef CONFIG_COMPAT" checks in this file and rely on
> > dead code elimination to remove the rest, but there might
> > be additional problems if some extern declarations are
> > hidden in an #ifdef as well.
I could remove all CONFIG_COMPAT ifdefs from this file, and for compiling
it required a few extra defines (in other files) to be moved outside of
their #ifdef CONFIG_COMPAT. Those being:
#define VFP_STATE_SIZE ((32 * 8) + 4)
#define VFP_FPSCR_STAT_MASK 0xf800009f
#define VFP_FPSCR_CTRL_MASK 0x07f79f00
#define COMPAT_ELF_NGREG 18
typedef unsigned int compat_elf_greg_t;
typedef compat_elf_greg_t compat_elf_gregset_t[COMPAT_ELF_NGREG];
OTOH, the size of the final arch/arm64/kernel/ptrace.o went from 44768 to
56328 bytes, which I understand to be undesired.
A different (and simpler) solution is to have an empty struct in case of
!CONFIG_COMPAT, that will be optimized out in compile-time:
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 9f8781f1fdfda..d2f275d8a3e6e 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -2107,6 +2107,9 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
return ret;
}
+#else
+static const struct user_regset_view user_aarch32_view = {};
+static const struct user_regset_view user_aarch32_ptrace_view = {};
#endif /* CONFIG_COMPAT */
const struct user_regset_view *task_user_regset_view(struct task_struct *task)
With this the patch will build successfully and arch/arm64/kernel/ptrace.o
will be able to keep it's original size.
Arnd, is that ok?
Thanks!
Leo
> >
> > Arnd
>
> Sure, I sill send a v2 soon.
>
> Thanks!
> Leo
>
> >
On Mon, Jan 8, 2024, at 17:04, Leonardo Bras wrote:
> On Mon, Jan 08, 2024 at 12:07:48PM -0300, Leonardo Bras wrote:
>> On Fri, Jan 05, 2024 at 03:38:05PM +0100, Arnd Bergmann wrote:
>> >
>> > I suspect it's enough to remove all of the other
>> > "#ifdef CONFIG_COMPAT" checks in this file and rely on
>> > dead code elimination to remove the rest, but there might
>> > be additional problems if some extern declarations are
>> > hidden in an #ifdef as well.
>
> I could remove all CONFIG_COMPAT ifdefs from this file, and for compiling
> it required a few extra defines (in other files) to be moved outside of
> their #ifdef CONFIG_COMPAT. Those being:
>
> #define VFP_STATE_SIZE ((32 * 8) + 4)
> #define VFP_FPSCR_STAT_MASK 0xf800009f
> #define VFP_FPSCR_CTRL_MASK 0x07f79f00
>
> #define COMPAT_ELF_NGREG 18
> typedef unsigned int compat_elf_greg_t;
> typedef compat_elf_greg_t compat_elf_gregset_t[COMPAT_ELF_NGREG];
>
>
> OTOH, the size of the final arch/arm64/kernel/ptrace.o went from 44768 to
> 56328 bytes, which I understand to be undesired.
Right, unfortunately it seems that compat_arch_ptrace() is
globally visible and consequently not dropped by the compiler
in dead code elimination.
> A different (and simpler) solution is to have an empty struct in case of
> !CONFIG_COMPAT, that will be optimized out in compile-time:
>
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index 9f8781f1fdfda..d2f275d8a3e6e 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -2107,6 +2107,9 @@ long compat_arch_ptrace(struct task_struct
> *child, compat_long_t request,
>
> return ret;
> }
> +#else
> +static const struct user_regset_view user_aarch32_view = {};
> +static const struct user_regset_view user_aarch32_ptrace_view = {};
> #endif /* CONFIG_COMPAT */
>
> const struct user_regset_view *task_user_regset_view(struct task_struct *task)
>
> With this the patch will build successfully and arch/arm64/kernel/ptrace.o
> will be able to keep it's original size.
>
> Arnd, is that ok?
I don't see it being worth it if you add extra unused lines
in order to remove one more #ifdef. I would either leave the
task_user_regset_view() function unchanged here, or (if this
works) move the #ifdef down a few lines so the existing
user_regset_view structures can be shared:
@@ -1595,7 +1595,6 @@ static const struct user_regset_view user_aarch64_view = {
.regsets = aarch64_regsets, .n = ARRAY_SIZE(aarch64_regsets)
};
-#ifdef CONFIG_COMPAT
enum compat_regset {
REGSET_COMPAT_GPR,
REGSET_COMPAT_VFP,
@@ -1852,6 +1851,7 @@ static const struct user_regset_view user_aarch32_ptrace_view = {
.regsets = aarch32_ptrace_regsets, .n = ARRAY_SIZE(aarch32_ptrace_regsets)
};
+#ifdef CONFIG_COMPAT
static int compat_ptrace_read_user(struct task_struct *tsk, compat_ulong_t off,
compat_ulong_t __user *ret)
{
Arnd
On Mon, Jan 08, 2024 at 05:26:26PM +0100, Arnd Bergmann wrote:
> On Mon, Jan 8, 2024, at 17:04, Leonardo Bras wrote:
> > On Mon, Jan 08, 2024 at 12:07:48PM -0300, Leonardo Bras wrote:
> >> On Fri, Jan 05, 2024 at 03:38:05PM +0100, Arnd Bergmann wrote:
> >> >
> >> > I suspect it's enough to remove all of the other
> >> > "#ifdef CONFIG_COMPAT" checks in this file and rely on
> >> > dead code elimination to remove the rest, but there might
> >> > be additional problems if some extern declarations are
> >> > hidden in an #ifdef as well.
> >
> > I could remove all CONFIG_COMPAT ifdefs from this file, and for compiling
> > it required a few extra defines (in other files) to be moved outside of
> > their #ifdef CONFIG_COMPAT. Those being:
> >
> > #define VFP_STATE_SIZE ((32 * 8) + 4)
> > #define VFP_FPSCR_STAT_MASK 0xf800009f
> > #define VFP_FPSCR_CTRL_MASK 0x07f79f00
> >
> > #define COMPAT_ELF_NGREG 18
> > typedef unsigned int compat_elf_greg_t;
> > typedef compat_elf_greg_t compat_elf_gregset_t[COMPAT_ELF_NGREG];
> >
> >
> > OTOH, the size of the final arch/arm64/kernel/ptrace.o went from 44768 to
> > 56328 bytes, which I understand to be undesired.
>
> Right, unfortunately it seems that compat_arch_ptrace() is
> globally visible and consequently not dropped by the compiler
> in dead code elimination.
>
> > A different (and simpler) solution is to have an empty struct in case of
> > !CONFIG_COMPAT, that will be optimized out in compile-time:
> >
> > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> > index 9f8781f1fdfda..d2f275d8a3e6e 100644
> > --- a/arch/arm64/kernel/ptrace.c
> > +++ b/arch/arm64/kernel/ptrace.c
> > @@ -2107,6 +2107,9 @@ long compat_arch_ptrace(struct task_struct
> > *child, compat_long_t request,
> >
> > return ret;
> > }
> > +#else
> > +static const struct user_regset_view user_aarch32_view = {};
> > +static const struct user_regset_view user_aarch32_ptrace_view = {};
> > #endif /* CONFIG_COMPAT */
> >
> > const struct user_regset_view *task_user_regset_view(struct task_struct *task)
> >
> > With this the patch will build successfully and arch/arm64/kernel/ptrace.o
> > will be able to keep it's original size.
> >
> > Arnd, is that ok?
>
> I don't see it being worth it if you add extra unused lines
> in order to remove one more #ifdef. I would either leave the
> task_user_regset_view() function unchanged here, or (if this
> works) move the #ifdef down a few lines so the existing
> user_regset_view structures can be shared:
>
> @@ -1595,7 +1595,6 @@ static const struct user_regset_view user_aarch64_view = {
> .regsets = aarch64_regsets, .n = ARRAY_SIZE(aarch64_regsets)
> };
>
> -#ifdef CONFIG_COMPAT
> enum compat_regset {
> REGSET_COMPAT_GPR,
> REGSET_COMPAT_VFP,
> @@ -1852,6 +1851,7 @@ static const struct user_regset_view user_aarch32_ptrace_view = {
> .regsets = aarch32_ptrace_regsets, .n = ARRAY_SIZE(aarch32_ptrace_regsets)
> };
>
> +#ifdef CONFIG_COMPAT
> static int compat_ptrace_read_user(struct task_struct *tsk, compat_ulong_t off,
> compat_ulong_t __user *ret)
> {
>
Ok, with the previous defines moved outside !CONFIG_COMPAT, this works as a
charm and keeps arch/arm64/kernel/ptrace.o with the same size.
I will send a v2 soon.
Thanks!
Leo
>
> Arnd
>