linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: linux@arm.linux.org.uk (Russell King - ARM Linux)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v6 00/14] uprobes: Add uprobes support for ARM
Date: Sat, 1 Mar 2014 12:30:26 +0000	[thread overview]
Message-ID: <20140301123026.GD21483@n2100.arm.linux.org.uk> (raw)
In-Reply-To: <1392017945-4507-1-git-send-email-dave.long@linaro.org>

On Mon, Feb 10, 2014 at 02:38:51AM -0500, David Long wrote:
> This patch series adds basic uprobes support to ARM. It is based on patches
> developed earlier by Rabin Vincent. That approach of adding hooks into
> the kprobes instruction parsing code was not well received. This approach
> separates the ARM instruction parsing code in kprobes out into a separate set
> of functions which can be used by both kprobes and uprobes. Both kprobes and
> uprobes then provide their own semantic action tables to process the results of
> the parsing.

Here's more build errors, from omap4430-sdp's randconfig build from
last night - note that the linker can't always get the filenames
right:

arch/arm/kernel/built-in.o: In function `do_work_pending':
psci_smp.c:(.text+0x2e44): undefined reference to `uprobe_notify_resume'
arch/arm/kernel/built-in.o: In function `uprobe_trap_handler':
psci_smp.c:(.text+0x79d4): undefined reference to `uprobe_pre_sstep_notifier'
psci_smp.c:(.text+0x79e8): undefined reference to `uprobe_post_sstep_notifier'
arch/arm/kernel/built-in.o: In function `set_swbp':
psci_smp.c:(.text+0x7a54): undefined reference to `uprobe_write_opcode'
kernel/built-in.o: In function `mmput.part.45':
context_tracking.c:(.text+0xaf0): undefined reference to `uprobe_clear_state'
kernel/built-in.o: In function `mm_release':
context_tracking.c:(.text+0xd3c): undefined reference to `uprobe_free_utask'
kernel/built-in.o: In function `copy_process.part.47':
context_tracking.c:(.text+0x1748): undefined reference to `uprobe_copy_process'
kernel/built-in.o: In function `get_signal_to_deliver':
context_tracking.c:(.text+0x111bc): undefined reference to `uprobe_deny_signal'
kernel/built-in.o: In function `probe_event_enable.constprop.5':
context_tracking.c:(.text+0x9f0b8): undefined reference to `uprobe_register'
kernel/built-in.o: In function `trace_uprobe_register':
context_tracking.c:(.text+0x9f134): undefined reference to `uprobe_unregister'
kernel/built-in.o: In function `dup_mmap':
context_tracking.c:(.text.unlikely+0x354): undefined reference to `uprobe_start_dup_mmap'
context_tracking.c:(.text.unlikely+0x380): undefined reference to `uprobe_dup_mmap'
context_tracking.c:(.text.unlikely+0x5e8): undefined reference to `uprobe_end_dup_mmap'
mm/built-in.o: In function `unmap_single_vma':
page_isolation.c:(.text+0x22d10): undefined reference to `uprobe_munmap'
mm/built-in.o: In function `vma_adjust':
page_isolation.c:(.text+0x2748c): undefined reference to `uprobe_munmap'
page_isolation.c:(.text+0x274a0): undefined reference to `uprobe_munmap'
page_isolation.c:(.text+0x27858): undefined reference to `uprobe_mmap'
page_isolation.c:(.text+0x2786c): undefined reference to `uprobe_mmap'
page_isolation.c:(.text+0x2788c): undefined reference to `uprobe_munmap'
page_isolation.c:(.text+0x2790c): undefined reference to `uprobe_mmap'
mm/built-in.o: In function `mmap_region':
page_isolation.c:(.text+0x2963c): undefined reference to `uprobe_mmap'
make[1]: *** [vmlinux] Error 1
make[1]: Target `zImage' not remade because of errors.

So, let's take uprobe_notify_resume().  This is unconditionally referenced
by arch/arm/kernel/signal.c:

                                syscall = 0;
                        } else if (thread_flags & _TIF_UPROBE) {
                                clear_thread_flag(TIF_UPROBE);
                                uprobe_notify_resume(regs);
                        } else {
                                clear_thread_flag(TIF_NOTIFY_RESUME);
                                tracehook_notify_resume(regs);
                        }

and is declared in include/linux/uprobes.h:

	#ifdef CONFIG_UPROBES
	#include <asm/uprobes.h>
	...
	extern void uprobe_notify_resume(struct pt_regs *regs);
	...
	#else /* !CONFIG_UPROBES */
	...
	static inline void uprobe_notify_resume(struct pt_regs *regs)
	{
	}
	...
	#endif /* !CONFIG_UPROBES */

It is defined in kernel/events/uprobes.c:

	kernel/events/uprobes.c:void uprobe_notify_resume(struct pt_regs *regs)

which is built when CONFIG_UPROBES is set:

	obj-$(CONFIG_UPROBES) += uprobes.o

but, the events directory is only built when CONFIG_PERF_EVENTS is also
set:

	obj-$(CONFIG_PERF_EVENTS) += events/

and the failing configuration has:

	# CONFIG_PERF_EVENTS is not set
	CONFIG_UPROBES=y

Clearly, it doesn't make sense for UPROBES to be enabled with PERF_EVENTS
disabled - and indeed the Kconfig ensures that this dependency is properly
expressed:

	config UPROBES
	        bool "Transparent user-space probes (EXPERIMENTAL)"
	        depends on UPROBE_EVENT && PERF_EVENTS
	        default n
	        select PERCPU_RWSEM

but where this all falls down is here:

	config UPROBE_EVENT
	        bool "Enable uprobes-based dynamic events"
	        depends on ARCH_SUPPORTS_UPROBES
	        depends on MMU
	        select UPROBES
	        select PROBE_EVENTS
	        select TRACING
	        default n

Which is yet another brilliant example of why this "select" crap is soo
evil.  Yes, the failing configuration has:

	CONFIG_UPROBE_EVENT=y

Ineed, there was a Kconfig warning:

	warning: (UPROBE_EVENT) selects UPROBES which has unmet direct dependencies (UPROBE_EVENT && PERF_EVENTS)

This is not your fault.  It's the fault of everyone who passed through
commit f3f096cfedf8113380c56fc855275cc75cd8cf55 without properly reviewing
it and paying attention to that select crap.  Given how evil "select" is,
it's something which should always be thoroughly reviewed - with analysis
of the dependencies.  I believe commits which introduce new select
statements should document an analysis of why those new select statements
are appropriate and how they ensure that any dependencies of the selected
symbol are not violated.

Therefore, I will not take the ARM uprobes code while this kind of
select abortion is present - it needs to be fixed first to avoid these
build errors.  Sorry.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

  parent reply	other threads:[~2014-03-01 12:30 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-02-10  7:38 [PATCH v6 00/14] uprobes: Add uprobes support for ARM David Long
2014-02-10  7:38 ` [PATCH v6 01/14] uprobes: allow ignoring of probe hits David Long
2014-02-10  7:38 ` [PATCH v6 02/14] ARM: move shared uprobe/kprobe definitions into new include file David Long
2014-02-10  7:38 ` [PATCH v6 03/14] ARM: Move generic arm instruction parsing code to new files for sharing between features David Long
2014-02-10  7:38 ` [PATCH v6 04/14] ARM: move generic thumb instruction parsing code to new files for use by other feature David Long
2014-02-10  7:38 ` [PATCH v6 05/14] ARM: use a function table for determining instruction interpreter action David Long
2014-02-10  7:38 ` [PATCH v6 06/14] ARM: Disable jprobes test when built into thumb-mode kernel David Long
2014-02-10  7:38 ` [PATCH v6 07/14] ARM: Remove use of struct kprobe from generic probes code David Long
2014-02-28 10:12   ` Russell King - ARM Linux
2014-02-28 14:11     ` Jon Medhurst (Tixy)
2014-02-28 14:45       ` Jon Medhurst (Tixy)
2014-03-02 10:37     ` David Long
2014-03-02 12:11       ` Russell King - ARM Linux
2014-02-10  7:38 ` [PATCH v6 08/14] ARM: Make the kprobes condition_check symbol names more generic David Long
2014-02-10  7:39 ` [PATCH v6 09/14] ARM: Change more ARM kprobes symbol names to something more David Long
2014-02-10  7:39 ` [PATCH v6 10/14] ARM: Rename the shared kprobes/uprobe return value enum David Long
2014-02-10  7:39 ` [PATCH v6 11/14] ARM: Change the remaining shared kprobes/uprobes symbols to something generic David Long
2014-02-10  7:39 ` [PATCH v6 12/14] ARM: Add an emulate flag to the kprobes/uprobes instruction decode functions David Long
2014-02-10  7:39 ` [PATCH v6 13/14] ARM: Make arch_specific_insn a define for new arch_probes_insn structure David Long
2014-02-10  7:39 ` [PATCH v6 14/14] ARM: add uprobes support David Long
2014-03-01 12:30 ` Russell King - ARM Linux [this message]
2014-03-02 12:02   ` [PATCH v6 00/14] uprobes: Add uprobes support for ARM David Long
2014-03-03  6:23     ` Srikar Dronamraju
2014-03-03 10:08       ` David Long
2014-03-03 10:39       ` Russell King - ARM Linux
2014-03-03 20:50     ` Oleg Nesterov
2014-03-04  0:53       ` Russell King - ARM Linux
2014-03-04 17:31         ` Oleg Nesterov
2014-03-06  8:10           ` David Long

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140301123026.GD21483@n2100.arm.linux.org.uk \
    --to=linux@arm.linux.org.uk \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).