* [PATCH V4 4/4] Adjust objdump start/end range per map pgoff parameter
2024-08-28 5:09 [PATCH V4 0/4] arm-cs-trace-disasm.py/perf must accommodate non-zero DSO text offset Steve Clevenger
@ 2024-08-28 5:09 ` Steve Clevenger
2024-08-28 8:46 ` Leo Yan
2024-08-28 5:09 ` [PATCH V4 3/4] Add map pgoff to python dictionary based on MAPPING_TYPE Steve Clevenger
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Steve Clevenger @ 2024-08-28 5:09 UTC (permalink / raw)
To: james.clark, mike.leach
Cc: suzuki.poulose, leo.yan, ilkka, coresight, linux-perf-users,
linux-arm-kernel
Extract map_pgoff parameter from the dictionary, and adjust start/end
range passed to objdump based on the value.
The start_addr/stop_addr address checks are changed to print a warning
only if verbose == True. This script repeatedly sees a zero value passed
in for
start_addr = cpu_data[str(cpu) + 'addr']
These zero values are not a new problem. The start_addr/stop_addr warning
clutters the instruction trace output, hence this change.
Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com>
---
tools/perf/scripts/python/arm-cs-trace-disasm.py | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/tools/perf/scripts/python/arm-cs-trace-disasm.py b/tools/perf/scripts/python/arm-cs-trace-disasm.py
index 7aff02d84ffb..e8cf5d80d850 100755
--- a/tools/perf/scripts/python/arm-cs-trace-disasm.py
+++ b/tools/perf/scripts/python/arm-cs-trace-disasm.py
@@ -187,6 +187,7 @@ def process_event(param_dict):
dso_start = get_optional(param_dict, "dso_map_start")
dso_end = get_optional(param_dict, "dso_map_end")
symbol = get_optional(param_dict, "symbol")
+ map_pgoff = get_optional(param_dict, "map_pgoff")
cpu = sample["cpu"]
ip = sample["ip"]
@@ -249,11 +250,13 @@ def process_event(param_dict):
return
if (start_addr < int(dso_start) or start_addr > int(dso_end)):
- print("Start address 0x%x is out of range [ 0x%x .. 0x%x ] for dso %s" % (start_addr, int(dso_start), int(dso_end), dso))
+ if (options.verbose == True):
+ print("Start address 0x%x is out of range [ 0x%x .. 0x%x ] for dso %s" % (start_addr, int(dso_start), int(dso_end), dso))
return
if (stop_addr < int(dso_start) or stop_addr > int(dso_end)):
- print("Stop address 0x%x is out of range [ 0x%x .. 0x%x ] for dso %s" % (stop_addr, int(dso_start), int(dso_end), dso))
+ if (options.verbose == True):
+ print("Stop address 0x%x is out of range [ 0x%x .. 0x%x ] for dso %s" % (stop_addr, int(dso_start), int(dso_end), dso))
return
if (options.objdump_name != None):
@@ -267,7 +270,7 @@ def process_event(param_dict):
dso_fname = get_dso_file_path(dso, dso_bid)
if path.exists(dso_fname):
- print_disam(dso_fname, dso_vm_start, start_addr, stop_addr)
+ print_disam(dso_fname, dso_vm_start, start_addr + map_pgoff, stop_addr + map_pgoff)
else:
print("Failed to find dso %s for address range [ 0x%x .. 0x%x ]" % (dso, start_addr, stop_addr))
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH V4 4/4] Adjust objdump start/end range per map pgoff parameter
2024-08-28 5:09 ` [PATCH V4 4/4] Adjust objdump start/end range per map pgoff parameter Steve Clevenger
@ 2024-08-28 8:46 ` Leo Yan
0 siblings, 0 replies; 13+ messages in thread
From: Leo Yan @ 2024-08-28 8:46 UTC (permalink / raw)
To: Steve Clevenger, james.clark, mike.leach
Cc: suzuki.poulose, ilkka, coresight, linux-perf-users,
linux-arm-kernel
On 8/28/2024 6:09 AM, Steve Clevenger wrote:
>
> Extract map_pgoff parameter from the dictionary, and adjust start/end
> range passed to objdump based on the value.
>
> The start_addr/stop_addr address checks are changed to print a warning
> only if verbose == True. This script repeatedly sees a zero value passed
> in for
> start_addr = cpu_data[str(cpu) + 'addr']
>
> These zero values are not a new problem. The start_addr/stop_addr warning
> clutters the instruction trace output, hence this change.
>
> Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com>
Reviewed-by: Leo Yan <leo.yan@arm.com>
> ---
> tools/perf/scripts/python/arm-cs-trace-disasm.py | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/scripts/python/arm-cs-trace-disasm.py b/tools/perf/scripts/python/arm-cs-trace-disasm.py
> index 7aff02d84ffb..e8cf5d80d850 100755
> --- a/tools/perf/scripts/python/arm-cs-trace-disasm.py
> +++ b/tools/perf/scripts/python/arm-cs-trace-disasm.py
> @@ -187,6 +187,7 @@ def process_event(param_dict):
> dso_start = get_optional(param_dict, "dso_map_start")
> dso_end = get_optional(param_dict, "dso_map_end")
> symbol = get_optional(param_dict, "symbol")
> + map_pgoff = get_optional(param_dict, "map_pgoff")
>
> cpu = sample["cpu"]
> ip = sample["ip"]
> @@ -249,11 +250,13 @@ def process_event(param_dict):
> return
>
> if (start_addr < int(dso_start) or start_addr > int(dso_end)):
> - print("Start address 0x%x is out of range [ 0x%x .. 0x%x ] for dso %s" % (start_addr, int(dso_start), int(dso_end), dso))
> + if (options.verbose == True):
> + print("Start address 0x%x is out of range [ 0x%x .. 0x%x ] for dso %s" % (start_addr, int(dso_start), int(dso_end), dso))
> return
>
> if (stop_addr < int(dso_start) or stop_addr > int(dso_end)):
> - print("Stop address 0x%x is out of range [ 0x%x .. 0x%x ] for dso %s" % (stop_addr, int(dso_start), int(dso_end), dso))
> + if (options.verbose == True):
> + print("Stop address 0x%x is out of range [ 0x%x .. 0x%x ] for dso %s" % (stop_addr, int(dso_start), int(dso_end), dso))
> return
>
> if (options.objdump_name != None):
> @@ -267,7 +270,7 @@ def process_event(param_dict):
>
> dso_fname = get_dso_file_path(dso, dso_bid)
> if path.exists(dso_fname):
> - print_disam(dso_fname, dso_vm_start, start_addr, stop_addr)
> + print_disam(dso_fname, dso_vm_start, start_addr + map_pgoff, stop_addr + map_pgoff)
> else:
> print("Failed to find dso %s for address range [ 0x%x .. 0x%x ]" % (dso, start_addr, stop_addr))
>
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH V4 3/4] Add map pgoff to python dictionary based on MAPPING_TYPE
2024-08-28 5:09 [PATCH V4 0/4] arm-cs-trace-disasm.py/perf must accommodate non-zero DSO text offset Steve Clevenger
2024-08-28 5:09 ` [PATCH V4 4/4] Adjust objdump start/end range per map pgoff parameter Steve Clevenger
@ 2024-08-28 5:09 ` Steve Clevenger
2024-08-28 8:44 ` Leo Yan
2024-08-28 5:09 ` [PATCH V4 2/4] Force MAPPING_TYPE__IDENTIY for PIE Steve Clevenger
2024-08-28 5:09 ` [PATCH V4 1/4] Add dso__is_pie call to identify ELF PIE Steve Clevenger
3 siblings, 1 reply; 13+ messages in thread
From: Steve Clevenger @ 2024-08-28 5:09 UTC (permalink / raw)
To: james.clark, mike.leach
Cc: suzuki.poulose, leo.yan, ilkka, coresight, linux-perf-users,
linux-arm-kernel
Add map_pgoff parameter to python dictionary so it can be seen by the
python script, arm-cs-trace-disasm.py. map_pgoff is forced to zero in
the dictionary if file type is MAPPING_TYPE__IDENTITY. Otherwise, the
map_pgoff value is directly added to the dictionary.
Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com>
---
.../util/scripting-engines/trace-event-python.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
index 6971dd6c231f..b8da0ea5e55c 100644
--- a/tools/perf/util/scripting-engines/trace-event-python.c
+++ b/tools/perf/util/scripting-engines/trace-event-python.c
@@ -798,7 +798,8 @@ static int set_regs_in_dict(PyObject *dict,
static void set_sym_in_dict(PyObject *dict, struct addr_location *al,
const char *dso_field, const char *dso_bid_field,
const char *dso_map_start, const char *dso_map_end,
- const char *sym_field, const char *symoff_field)
+ const char *sym_field, const char *symoff_field,
+ const char *map_pgoff)
{
char sbuild_id[SBUILD_ID_SIZE];
@@ -814,6 +815,12 @@ static void set_sym_in_dict(PyObject *dict, struct addr_location *al,
PyLong_FromUnsignedLong(map__start(al->map)));
pydict_set_item_string_decref(dict, dso_map_end,
PyLong_FromUnsignedLong(map__end(al->map)));
+ if (al->map->mapping_type == MAPPING_TYPE__DSO)
+ pydict_set_item_string_decref(dict, map_pgoff,
+ PyLong_FromUnsignedLongLong(al->map->pgoff));
+ else
+ pydict_set_item_string_decref(dict, map_pgoff,
+ PyLong_FromUnsignedLongLong(0));
}
if (al->sym) {
pydict_set_item_string_decref(dict, sym_field,
@@ -900,7 +907,7 @@ static PyObject *get_perf_sample_dict(struct perf_sample *sample,
pydict_set_item_string_decref(dict, "comm",
_PyUnicode_FromString(thread__comm_str(al->thread)));
set_sym_in_dict(dict, al, "dso", "dso_bid", "dso_map_start", "dso_map_end",
- "symbol", "symoff");
+ "symbol", "symoff", "map_pgoff");
pydict_set_item_string_decref(dict, "callchain", callchain);
@@ -925,7 +932,7 @@ static PyObject *get_perf_sample_dict(struct perf_sample *sample,
PyBool_FromLong(1));
set_sym_in_dict(dict_sample, addr_al, "addr_dso", "addr_dso_bid",
"addr_dso_map_start", "addr_dso_map_end",
- "addr_symbol", "addr_symoff");
+ "addr_symbol", "addr_symoff", "map_pgoff");
}
if (sample->flags)
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH V4 3/4] Add map pgoff to python dictionary based on MAPPING_TYPE
2024-08-28 5:09 ` [PATCH V4 3/4] Add map pgoff to python dictionary based on MAPPING_TYPE Steve Clevenger
@ 2024-08-28 8:44 ` Leo Yan
2024-08-28 21:26 ` Steve Clevenger
0 siblings, 1 reply; 13+ messages in thread
From: Leo Yan @ 2024-08-28 8:44 UTC (permalink / raw)
To: Steve Clevenger, james.clark, mike.leach
Cc: suzuki.poulose, ilkka, coresight, linux-perf-users,
linux-arm-kernel
On 8/28/2024 6:09 AM, Steve Clevenger wrote:
>
> Add map_pgoff parameter to python dictionary so it can be seen by the
> python script, arm-cs-trace-disasm.py. map_pgoff is forced to zero in
> the dictionary if file type is MAPPING_TYPE__IDENTITY. Otherwise, the
> map_pgoff value is directly added to the dictionary.
>
> Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com>
> ---
> .../util/scripting-engines/trace-event-python.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
> index 6971dd6c231f..b8da0ea5e55c 100644
> --- a/tools/perf/util/scripting-engines/trace-event-python.c
> +++ b/tools/perf/util/scripting-engines/trace-event-python.c
> @@ -798,7 +798,8 @@ static int set_regs_in_dict(PyObject *dict,
> static void set_sym_in_dict(PyObject *dict, struct addr_location *al,
> const char *dso_field, const char *dso_bid_field,
> const char *dso_map_start, const char *dso_map_end,
> - const char *sym_field, const char *symoff_field)
> + const char *sym_field, const char *symoff_field,
> + const char *map_pgoff)
> {
> char sbuild_id[SBUILD_ID_SIZE];
>
> @@ -814,6 +815,12 @@ static void set_sym_in_dict(PyObject *dict, struct addr_location *al,
> PyLong_FromUnsignedLong(map__start(al->map)));
> pydict_set_item_string_decref(dict, dso_map_end,
> PyLong_FromUnsignedLong(map__end(al->map)));
> + if (al->map->mapping_type == MAPPING_TYPE__DSO)
> + pydict_set_item_string_decref(dict, map_pgoff,
> + PyLong_FromUnsignedLongLong(al->map->pgoff));
> + else
> + pydict_set_item_string_decref(dict, map_pgoff,
> + PyLong_FromUnsignedLongLong(0));
Indention is inconsistent. Please keep the same format.
> }
> if (al->sym) {
> pydict_set_item_string_decref(dict, sym_field,
> @@ -900,7 +907,7 @@ static PyObject *get_perf_sample_dict(struct perf_sample *sample,
> pydict_set_item_string_decref(dict, "comm",
> _PyUnicode_FromString(thread__comm_str(al->thread)));
> set_sym_in_dict(dict, al, "dso", "dso_bid", "dso_map_start", "dso_map_end",
> - "symbol", "symoff");
> + "symbol", "symoff", "map_pgoff");
>
> pydict_set_item_string_decref(dict, "callchain", callchain);
>
> @@ -925,7 +932,7 @@ static PyObject *get_perf_sample_dict(struct perf_sample *sample,
> PyBool_FromLong(1));
> set_sym_in_dict(dict_sample, addr_al, "addr_dso", "addr_dso_bid",
> "addr_dso_map_start", "addr_dso_map_end",
> - "addr_symbol", "addr_symoff");
> + "addr_symbol", "addr_symoff", "map_pgoff");
This dict variable is for data address parsing. For alignment the naming, here
can change it as "addr_map_pgoff"?
Please note, from my understanding, this renaming will not impact the
sequential change as the python script should only use the dso dict. Please
confirm at your side.
With above changes:
Reviewed-by: Leo Yan <leo.yan@arm.com>
> }
>
> if (sample->flags)
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH V4 3/4] Add map pgoff to python dictionary based on MAPPING_TYPE
2024-08-28 8:44 ` Leo Yan
@ 2024-08-28 21:26 ` Steve Clevenger
0 siblings, 0 replies; 13+ messages in thread
From: Steve Clevenger @ 2024-08-28 21:26 UTC (permalink / raw)
To: Leo Yan, james.clark, mike.leach
Cc: suzuki.poulose, ilkka, coresight, linux-perf-users,
linux-arm-kernel
On 8/28/2024 1:44 AM, Leo Yan wrote:
> On 8/28/2024 6:09 AM, Steve Clevenger wrote:
>>
>> Add map_pgoff parameter to python dictionary so it can be seen by the
>> python script, arm-cs-trace-disasm.py. map_pgoff is forced to zero in
>> the dictionary if file type is MAPPING_TYPE__IDENTITY. Otherwise, the
>> map_pgoff value is directly added to the dictionary.
>>
>> Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com>
>> ---
>> .../util/scripting-engines/trace-event-python.c | 13 ++++++++++---
>> 1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
>> index 6971dd6c231f..b8da0ea5e55c 100644
>> --- a/tools/perf/util/scripting-engines/trace-event-python.c
>> +++ b/tools/perf/util/scripting-engines/trace-event-python.c
>> @@ -798,7 +798,8 @@ static int set_regs_in_dict(PyObject *dict,
>> static void set_sym_in_dict(PyObject *dict, struct addr_location *al,
>> const char *dso_field, const char *dso_bid_field,
>> const char *dso_map_start, const char *dso_map_end,
>> - const char *sym_field, const char *symoff_field)
>> + const char *sym_field, const char *symoff_field,
>> + const char *map_pgoff)
>> {
>> char sbuild_id[SBUILD_ID_SIZE];
>>
>> @@ -814,6 +815,12 @@ static void set_sym_in_dict(PyObject *dict, struct addr_location *al,
>> PyLong_FromUnsignedLong(map__start(al->map)));
>> pydict_set_item_string_decref(dict, dso_map_end,
>> PyLong_FromUnsignedLong(map__end(al->map)));
>> + if (al->map->mapping_type == MAPPING_TYPE__DSO)
>> + pydict_set_item_string_decref(dict, map_pgoff,
>> + PyLong_FromUnsignedLongLong(al->map->pgoff));
>> + else
>> + pydict_set_item_string_decref(dict, map_pgoff,
>> + PyLong_FromUnsignedLongLong(0));
>
> Indention is inconsistent. Please keep the same format.
Corrected.
>
>> }
>> if (al->sym) {
>> pydict_set_item_string_decref(dict, sym_field,
>> @@ -900,7 +907,7 @@ static PyObject *get_perf_sample_dict(struct perf_sample *sample,
>> pydict_set_item_string_decref(dict, "comm",
>> _PyUnicode_FromString(thread__comm_str(al->thread)));
>> set_sym_in_dict(dict, al, "dso", "dso_bid", "dso_map_start", "dso_map_end",
>> - "symbol", "symoff");
>> + "symbol", "symoff", "map_pgoff");
>>
>> pydict_set_item_string_decref(dict, "callchain", callchain);
>>
>> @@ -925,7 +932,7 @@ static PyObject *get_perf_sample_dict(struct perf_sample *sample,
>> PyBool_FromLong(1));
>> set_sym_in_dict(dict_sample, addr_al, "addr_dso", "addr_dso_bid",
>> "addr_dso_map_start", "addr_dso_map_end",
>> - "addr_symbol", "addr_symoff");
>> + "addr_symbol", "addr_symoff", "map_pgoff");
>
> This dict variable is for data address parsing. For alignment the naming, here
> can change it as "addr_map_pgoff"?
>
> Please note, from my understanding, this renaming will not impact the
> sequential change as the python script should only use the dso dict. Please
> confirm at your side.
Renamed as "addr_map_pgoff" to match local call convention. The dso
dictionary is not affected. See V5 patch series.
>
> With above changes:
>
> Reviewed-by: Leo Yan <leo.yan@arm.com>
>
>> }
>>
>> if (sample->flags)
>> --
>> 2.25.1
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH V4 2/4] Force MAPPING_TYPE__IDENTIY for PIE
2024-08-28 5:09 [PATCH V4 0/4] arm-cs-trace-disasm.py/perf must accommodate non-zero DSO text offset Steve Clevenger
2024-08-28 5:09 ` [PATCH V4 4/4] Adjust objdump start/end range per map pgoff parameter Steve Clevenger
2024-08-28 5:09 ` [PATCH V4 3/4] Add map pgoff to python dictionary based on MAPPING_TYPE Steve Clevenger
@ 2024-08-28 5:09 ` Steve Clevenger
2024-08-28 8:26 ` Leo Yan
2024-08-28 5:09 ` [PATCH V4 1/4] Add dso__is_pie call to identify ELF PIE Steve Clevenger
3 siblings, 1 reply; 13+ messages in thread
From: Steve Clevenger @ 2024-08-28 5:09 UTC (permalink / raw)
To: james.clark, mike.leach
Cc: suzuki.poulose, leo.yan, ilkka, coresight, linux-perf-users,
linux-arm-kernel
Use dso__is_pie() to check whether the DSO file is a Position
Independent Executable (PIE). If PIE, change the MAPPING_TYPE to
MAPPING_TYPE__IDENTITY so a zero map pgoff (text offset) is passed
into the script.
Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com>
---
tools/perf/util/map.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index e781c8d56a9a..c846faec177b 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -173,8 +173,8 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
assert(!dso__kernel(dso));
map__init(result, start, start + len, pgoff, dso, prot, flags);
- if (anon || no_dso) {
- map->mapping_type = MAPPING_TYPE__IDENTITY;
+ if (anon || no_dso || dso__is_pie(dso)) {
+ map__set_mapping_type(map, MAPPING_TYPE__IDENTITY);
/*
* Set memory without DSO as loaded. All map__find_*
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH V4 2/4] Force MAPPING_TYPE__IDENTIY for PIE
2024-08-28 5:09 ` [PATCH V4 2/4] Force MAPPING_TYPE__IDENTIY for PIE Steve Clevenger
@ 2024-08-28 8:26 ` Leo Yan
0 siblings, 0 replies; 13+ messages in thread
From: Leo Yan @ 2024-08-28 8:26 UTC (permalink / raw)
To: Steve Clevenger, james.clark, mike.leach
Cc: suzuki.poulose, ilkka, coresight, linux-perf-users,
linux-arm-kernel
On 8/28/2024 6:09 AM, Steve Clevenger wrote:
>
> Use dso__is_pie() to check whether the DSO file is a Position
> Independent Executable (PIE). If PIE, change the MAPPING_TYPE to
> MAPPING_TYPE__IDENTITY so a zero map pgoff (text offset) is passed
> into the script.
>
> Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com>
Reviewed-by: Leo Yan <leo.yan@arm.com>
> ---
> tools/perf/util/map.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index e781c8d56a9a..c846faec177b 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -173,8 +173,8 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
> assert(!dso__kernel(dso));
> map__init(result, start, start + len, pgoff, dso, prot, flags);
>
> - if (anon || no_dso) {
> - map->mapping_type = MAPPING_TYPE__IDENTITY;
> + if (anon || no_dso || dso__is_pie(dso)) {
> + map__set_mapping_type(map, MAPPING_TYPE__IDENTITY);
>
> /*
> * Set memory without DSO as loaded. All map__find_*
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH V4 1/4] Add dso__is_pie call to identify ELF PIE
2024-08-28 5:09 [PATCH V4 0/4] arm-cs-trace-disasm.py/perf must accommodate non-zero DSO text offset Steve Clevenger
` (2 preceding siblings ...)
2024-08-28 5:09 ` [PATCH V4 2/4] Force MAPPING_TYPE__IDENTIY for PIE Steve Clevenger
@ 2024-08-28 5:09 ` Steve Clevenger
2024-08-28 8:23 ` Leo Yan
3 siblings, 1 reply; 13+ messages in thread
From: Steve Clevenger @ 2024-08-28 5:09 UTC (permalink / raw)
To: james.clark, mike.leach
Cc: suzuki.poulose, leo.yan, ilkka, coresight, linux-perf-users,
linux-arm-kernel
Add dso__is_pie global to read the .dynamic section DT_FLAGS_1 entry for
the DF_1_PIE flag. This identifies position executable code.
Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com>
---
tools/perf/util/symbol-elf.c | 60 ++++++++++++++++++++++++++++++++++++
tools/perf/util/symbol.h | 1 +
2 files changed, 61 insertions(+)
diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index e398abfd13a0..0e49c1345a67 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -662,6 +662,66 @@ static int dso__synthesize_plt_got_symbols(struct dso *dso, Elf *elf,
return err;
}
+/*
+ * Check dynamic section DT_FLAGS_1 for a Position Independent
+ * Executable (PIE).
+ */
+bool dso__is_pie(struct dso *dso)
+{
+ Elf *elf = NULL;
+ Elf_Scn *scn = NULL;
+ GElf_Ehdr ehdr;
+ GElf_Shdr shdr;
+ bool is_pie = false;
+ char dso_path[PATH_MAX];
+ int fd = -1;
+
+ if (!dso || (elf_version(EV_CURRENT) == EV_NONE))
+ goto exit; // false
+
+ dso__build_id_filename(dso, dso_path, sizeof(dso_path), false);
+
+ fd = open(dso_path, O_RDONLY);
+
+ if (fd < 0) {
+ pr_debug("%s: cannot read cached %s.\n", __func__, dso_path);
+ goto exit; // false
+ }
+
+ elf = elf_begin(fd, ELF_C_READ, NULL);
+ gelf_getehdr(elf, &ehdr);
+
+ if (ehdr.e_type == ET_DYN) {
+ Elf_Data *data;
+ GElf_Dyn *entry;
+ int n_entries = shdr.sh_size / sizeof(GElf_Dyn);
+
+ scn = elf_section_by_name(elf, &ehdr, &shdr, ".dynamic", NULL);
+ if (!scn)
+ goto exit; // false
+
+ data = (Elf_Data *) elf_getdata(scn, NULL);
+ if (!data || !data->d_buf)
+ goto exit; // false
+
+ // check DT_FLAGS_1
+ for (int i = 0; i < n_entries; i++) {
+ entry = ((GElf_Dyn *) data->d_buf) + i;
+ if (entry->d_tag == DT_FLAGS_1) {
+ if ((entry->d_un.d_val & DF_1_PIE) != 0) {
+ is_pie = true;
+ break;
+ }
+ }
+ } // end for
+ }
+
+ elf_end(elf);
+ close(fd);
+exit:
+ return is_pie;
+}
+
/*
* We need to check if we have a .dynsym, so that we can handle the
* .plt, synthesizing its symbols, that aren't on the symtabs (be it
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 3fb5d146d9b1..33ea2596ce31 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -127,6 +127,7 @@ void dso__insert_symbol(struct dso *dso,
struct symbol *sym);
void dso__delete_symbol(struct dso *dso,
struct symbol *sym);
+bool dso__is_pie(struct dso *dso);
struct symbol *dso__find_symbol(struct dso *dso, u64 addr);
struct symbol *dso__find_symbol_nocache(struct dso *dso, u64 addr);
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH V4 1/4] Add dso__is_pie call to identify ELF PIE
2024-08-28 5:09 ` [PATCH V4 1/4] Add dso__is_pie call to identify ELF PIE Steve Clevenger
@ 2024-08-28 8:23 ` Leo Yan
2024-08-28 15:48 ` Steve Clevenger
0 siblings, 1 reply; 13+ messages in thread
From: Leo Yan @ 2024-08-28 8:23 UTC (permalink / raw)
To: Steve Clevenger, james.clark, mike.leach
Cc: suzuki.poulose, ilkka, coresight, linux-perf-users,
linux-arm-kernel
On 8/28/2024 6:09 AM, Steve Clevenger wrote:
>
> Add dso__is_pie global to read the .dynamic section DT_FLAGS_1 entry for
> the DF_1_PIE flag. This identifies position executable code.
>
> Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com>
> ---
> tools/perf/util/symbol-elf.c | 60 ++++++++++++++++++++++++++++++++++++
> tools/perf/util/symbol.h | 1 +
> 2 files changed, 61 insertions(+)
>
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index e398abfd13a0..0e49c1345a67 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -662,6 +662,66 @@ static int dso__synthesize_plt_got_symbols(struct dso *dso, Elf *elf,
> return err;
> }
>
> +/*
> + * Check dynamic section DT_FLAGS_1 for a Position Independent
> + * Executable (PIE).
> + */
> +bool dso__is_pie(struct dso *dso)
> +{
> + Elf *elf = NULL;
> + Elf_Scn *scn = NULL;
> + GElf_Ehdr ehdr;
> + GElf_Shdr shdr;
> + bool is_pie = false;
> + char dso_path[PATH_MAX];
> + int fd = -1;
> +
> + if (!dso || (elf_version(EV_CURRENT) == EV_NONE))
> + goto exit; // false
> +
> + dso__build_id_filename(dso, dso_path, sizeof(dso_path), false);
> +
> + fd = open(dso_path, O_RDONLY);
> +
> + if (fd < 0) {
> + pr_debug("%s: cannot read cached %s.\n", __func__, dso_path);
> + goto exit; // false
> + }
> +
> + elf = elf_begin(fd, ELF_C_READ, NULL);
> + gelf_getehdr(elf, &ehdr);
> +
> + if (ehdr.e_type == ET_DYN) {
> + Elf_Data *data;
> + GElf_Dyn *entry;
> + int n_entries = shdr.sh_size / sizeof(GElf_Dyn);
> +
> + scn = elf_section_by_name(elf, &ehdr, &shdr, ".dynamic", NULL);
> + if (!scn)
> + goto exit; // false
It should goto a new tag (e.g. exit_failed) so can invoke elf_end(elf) and
close(fd).
> +
> + data = (Elf_Data *) elf_getdata(scn, NULL);
> + if (!data || !data->d_buf)
> + goto exit; // false
Ditto.
> +
> + // check DT_FLAGS_1
> + for (int i = 0; i < n_entries; i++) {
> + entry = ((GElf_Dyn *) data->d_buf) + i;
> + if (entry->d_tag == DT_FLAGS_1) {
> + if ((entry->d_un.d_val & DF_1_PIE) != 0) {
> + is_pie = true;
> + break;
> + }
> + }
> + } // end for
> + }
> +
Put 'exit_failed' tag at here.
With the changes:
Reviewed-by: Leo Yan <leo.yan@arm.com>
> + elf_end(elf);
> + close(fd);
> +exit:
> + return is_pie;
> +}
> +
> /*
> * We need to check if we have a .dynsym, so that we can handle the
> * .plt, synthesizing its symbols, that aren't on the symtabs (be it
> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> index 3fb5d146d9b1..33ea2596ce31 100644
> --- a/tools/perf/util/symbol.h
> +++ b/tools/perf/util/symbol.h
> @@ -127,6 +127,7 @@ void dso__insert_symbol(struct dso *dso,
> struct symbol *sym);
> void dso__delete_symbol(struct dso *dso,
> struct symbol *sym);
> +bool dso__is_pie(struct dso *dso);
>
> struct symbol *dso__find_symbol(struct dso *dso, u64 addr);
> struct symbol *dso__find_symbol_nocache(struct dso *dso, u64 addr);
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH V4 1/4] Add dso__is_pie call to identify ELF PIE
2024-08-28 8:23 ` Leo Yan
@ 2024-08-28 15:48 ` Steve Clevenger
2024-08-28 20:34 ` Leo Yan
2024-08-28 20:36 ` Leo Yan
0 siblings, 2 replies; 13+ messages in thread
From: Steve Clevenger @ 2024-08-28 15:48 UTC (permalink / raw)
To: Leo Yan, james.clark, mike.leach
Cc: suzuki.poulose, ilkka, coresight, linux-perf-users,
linux-arm-kernel
On 8/28/2024 1:23 AM, Leo Yan wrote:
> On 8/28/2024 6:09 AM, Steve Clevenger wrote:
>>
>> Add dso__is_pie global to read the .dynamic section DT_FLAGS_1 entry for
>> the DF_1_PIE flag. This identifies position executable code.
>>
>> Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com>
>> ---
>> tools/perf/util/symbol-elf.c | 60 ++++++++++++++++++++++++++++++++++++
>> tools/perf/util/symbol.h | 1 +
>> 2 files changed, 61 insertions(+)
>>
>> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
>> index e398abfd13a0..0e49c1345a67 100644
>> --- a/tools/perf/util/symbol-elf.c
>> +++ b/tools/perf/util/symbol-elf.c
>> @@ -662,6 +662,66 @@ static int dso__synthesize_plt_got_symbols(struct dso *dso, Elf *elf,
>> return err;
>> }
>>
>> +/*
>> + * Check dynamic section DT_FLAGS_1 for a Position Independent
>> + * Executable (PIE).
>> + */
>> +bool dso__is_pie(struct dso *dso)
>> +{
>> + Elf *elf = NULL;
>> + Elf_Scn *scn = NULL;
>> + GElf_Ehdr ehdr;
>> + GElf_Shdr shdr;
>> + bool is_pie = false;
>> + char dso_path[PATH_MAX];
>> + int fd = -1;
>> +
>> + if (!dso || (elf_version(EV_CURRENT) == EV_NONE))
>> + goto exit; // false
>> +
>> + dso__build_id_filename(dso, dso_path, sizeof(dso_path), false);
>> +
>> + fd = open(dso_path, O_RDONLY);
>> +
>> + if (fd < 0) {
>> + pr_debug("%s: cannot read cached %s.\n", __func__, dso_path);
>> + goto exit; // false
>> + }
>> +
>> + elf = elf_begin(fd, ELF_C_READ, NULL);
>> + gelf_getehdr(elf, &ehdr);
>> +
>> + if (ehdr.e_type == ET_DYN) {
>> + Elf_Data *data;
>> + GElf_Dyn *entry;
>> + int n_entries = shdr.sh_size / sizeof(GElf_Dyn);
>> +
>> + scn = elf_section_by_name(elf, &ehdr, &shdr, ".dynamic", NULL);
>> + if (!scn)
>> + goto exit; // false
>
> It should goto a new tag (e.g. exit_failed) so can invoke elf_end(elf) and
> close(fd).
>
>> +
>> + data = (Elf_Data *) elf_getdata(scn, NULL);
>> + if (!data || !data->d_buf)
>> + goto exit; // false
>
> Ditto.
>
>> +
>> + // check DT_FLAGS_1
>> + for (int i = 0; i < n_entries; i++) {
>> + entry = ((GElf_Dyn *) data->d_buf) + i;
>> + if (entry->d_tag == DT_FLAGS_1) {
>> + if ((entry->d_un.d_val & DF_1_PIE) != 0) {
>> + is_pie = true;
>> + break;
>> + }
>> + }
>> + } // end for
>> + }
>> +
>
> Put 'exit_failed' tag at here.
>
> With the changes:
>
> Reviewed-by: Leo Yan <leo.yan@arm.com>
>
>> + elf_end(elf);
>> + close(fd);
>> +exit:
>> + return is_pie;
>> +}
>> +
Hi Leo,
It's been a long time since I've seen goto's used as a rule rather than
an exception. I see it introduced the problem you've mentioned. Is it ok
to simply update the cover letter (patch 0), and this one (patch 1) as
V5? If I don't hear from you, I'll resubmit all.
Thanks,
Steve
>> /*
>> * We need to check if we have a .dynsym, so that we can handle the
>> * .plt, synthesizing its symbols, that aren't on the symtabs (be it
>> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
>> index 3fb5d146d9b1..33ea2596ce31 100644
>> --- a/tools/perf/util/symbol.h
>> +++ b/tools/perf/util/symbol.h
>> @@ -127,6 +127,7 @@ void dso__insert_symbol(struct dso *dso,
>> struct symbol *sym);
>> void dso__delete_symbol(struct dso *dso,
>> struct symbol *sym);
>> +bool dso__is_pie(struct dso *dso);
>>
>> struct symbol *dso__find_symbol(struct dso *dso, u64 addr);
>> struct symbol *dso__find_symbol_nocache(struct dso *dso, u64 addr);
>> --
>> 2.25.1
>>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH V4 1/4] Add dso__is_pie call to identify ELF PIE
2024-08-28 15:48 ` Steve Clevenger
@ 2024-08-28 20:34 ` Leo Yan
2024-08-28 20:36 ` Leo Yan
1 sibling, 0 replies; 13+ messages in thread
From: Leo Yan @ 2024-08-28 20:34 UTC (permalink / raw)
To: scclevenger, james.clark, mike.leach
Cc: suzuki.poulose, ilkka, coresight, linux-perf-users,
linux-arm-kernel
On 8/28/2024 4:48 PM, Steve Clevenger wrote:
>
> On 8/28/2024 1:23 AM, Leo Yan wrote:
>> On 8/28/2024 6:09 AM, Steve Clevenger wrote:
>>>
>>> Add dso__is_pie global to read the .dynamic section DT_FLAGS_1 entry for
>>> the DF_1_PIE flag. This identifies position executable code.
>>>
>>> Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com>
>>> ---
>>> tools/perf/util/symbol-elf.c | 60 ++++++++++++++++++++++++++++++++++++
>>> tools/perf/util/symbol.h | 1 +
>>> 2 files changed, 61 insertions(+)
>>>
>>> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
>>> index e398abfd13a0..0e49c1345a67 100644
>>> --- a/tools/perf/util/symbol-elf.c
>>> +++ b/tools/perf/util/symbol-elf.c
>>> @@ -662,6 +662,66 @@ static int dso__synthesize_plt_got_symbols(struct dso *dso, Elf *elf,
>>> return err;
>>> }
>>>
>>> +/*
>>> + * Check dynamic section DT_FLAGS_1 for a Position Independent
>>> + * Executable (PIE).
>>> + */
>>> +bool dso__is_pie(struct dso *dso)
>>> +{
>>> + Elf *elf = NULL;
>>> + Elf_Scn *scn = NULL;
>>> + GElf_Ehdr ehdr;
>>> + GElf_Shdr shdr;
>>> + bool is_pie = false;
>>> + char dso_path[PATH_MAX];
>>> + int fd = -1;
>>> +
>>> + if (!dso || (elf_version(EV_CURRENT) == EV_NONE))
>>> + goto exit; // false
>>> +
>>> + dso__build_id_filename(dso, dso_path, sizeof(dso_path), false);
>>> +
>>> + fd = open(dso_path, O_RDONLY);
>>> +
>>> + if (fd < 0) {
>>> + pr_debug("%s: cannot read cached %s.\n", __func__, dso_path);
>>> + goto exit; // false
>>> + }
>>> +
>>> + elf = elf_begin(fd, ELF_C_READ, NULL);
>>> + gelf_getehdr(elf, &ehdr);
>>> +
>>> + if (ehdr.e_type == ET_DYN) {
>>> + Elf_Data *data;
>>> + GElf_Dyn *entry;
>>> + int n_entries = shdr.sh_size / sizeof(GElf_Dyn);
>>> +
>>> + scn = elf_section_by_name(elf, &ehdr, &shdr, ".dynamic", NULL);
>>> + if (!scn)
>>> + goto exit; // false
>>
>> It should goto a new tag (e.g. exit_failed) so can invoke elf_end(elf) and
>> close(fd).
>>
>>> +
>>> + data = (Elf_Data *) elf_getdata(scn, NULL);
>>> + if (!data || !data->d_buf)
>>> + goto exit; // false
>>
>> Ditto.
>>
>>> +
>>> + // check DT_FLAGS_1
>>> + for (int i = 0; i < n_entries; i++) {
>>> + entry = ((GElf_Dyn *) data->d_buf) + i;
>>> + if (entry->d_tag == DT_FLAGS_1) {
>>> + if ((entry->d_un.d_val & DF_1_PIE) != 0) {
>>> + is_pie = true;
>>> + break;
>>> + }
>>> + }
>>> + } // end for
>>> + }
>>> +
>>
>> Put 'exit_failed' tag at here.
>>
>> With the changes:
>>
>> Reviewed-by: Leo Yan <leo.yan@arm.com>
>>
>>> + elf_end(elf);
>>> + close(fd);
>>> +exit:
>>> + return is_pie;
>>> +}
>>> +
>
> Hi Leo,
>
> It's been a long time since I've seen goto's used as a rule rather than
> an exception. I see it introduced the problem you've mentioned.
I am not sure if I understand this. In the Linux code, it is quite common to
use goto to handle failure cases (for releasing resources).
> Is it ok to simply update the cover letter (patch 0), and this one (patch 1)
> as V5? If I don't hear from you, I'll resubmit all.
I assume you also need to address patch 03? It is good to send the all patches
in one go, this would be easier for maintainers to pick up the whole series
(e.g. using b4).
Thanks,
Leo
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH V4 1/4] Add dso__is_pie call to identify ELF PIE
2024-08-28 15:48 ` Steve Clevenger
2024-08-28 20:34 ` Leo Yan
@ 2024-08-28 20:36 ` Leo Yan
1 sibling, 0 replies; 13+ messages in thread
From: Leo Yan @ 2024-08-28 20:36 UTC (permalink / raw)
To: scclevenger, james.clark, mike.leach
Cc: suzuki.poulose, ilkka, coresight, linux-perf-users,
linux-arm-kernel
On 8/28/2024 4:48 PM, Steve Clevenger wrote:
>
> On 8/28/2024 1:23 AM, Leo Yan wrote:
>> On 8/28/2024 6:09 AM, Steve Clevenger wrote:
>>>
>>> Add dso__is_pie global to read the .dynamic section DT_FLAGS_1 entry for
>>> the DF_1_PIE flag. This identifies position executable code.
>>>
>>> Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com>
>>> ---
>>> tools/perf/util/symbol-elf.c | 60 ++++++++++++++++++++++++++++++++++++
>>> tools/perf/util/symbol.h | 1 +
>>> 2 files changed, 61 insertions(+)
>>>
>>> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
>>> index e398abfd13a0..0e49c1345a67 100644
>>> --- a/tools/perf/util/symbol-elf.c
>>> +++ b/tools/perf/util/symbol-elf.c
>>> @@ -662,6 +662,66 @@ static int dso__synthesize_plt_got_symbols(struct dso *dso, Elf *elf,
>>> return err;
>>> }
>>>
>>> +/*
>>> + * Check dynamic section DT_FLAGS_1 for a Position Independent
>>> + * Executable (PIE).
>>> + */
>>> +bool dso__is_pie(struct dso *dso)
>>> +{
>>> + Elf *elf = NULL;
>>> + Elf_Scn *scn = NULL;
>>> + GElf_Ehdr ehdr;
>>> + GElf_Shdr shdr;
>>> + bool is_pie = false;
>>> + char dso_path[PATH_MAX];
>>> + int fd = -1;
>>> +
>>> + if (!dso || (elf_version(EV_CURRENT) == EV_NONE))
>>> + goto exit; // false
>>> +
>>> + dso__build_id_filename(dso, dso_path, sizeof(dso_path), false);
>>> +
>>> + fd = open(dso_path, O_RDONLY);
>>> +
>>> + if (fd < 0) {
>>> + pr_debug("%s: cannot read cached %s.\n", __func__, dso_path);
>>> + goto exit; // false
>>> + }
>>> +
>>> + elf = elf_begin(fd, ELF_C_READ, NULL);
>>> + gelf_getehdr(elf, &ehdr);
>>> +
>>> + if (ehdr.e_type == ET_DYN) {
>>> + Elf_Data *data;
>>> + GElf_Dyn *entry;
>>> + int n_entries = shdr.sh_size / sizeof(GElf_Dyn);
>>> +
>>> + scn = elf_section_by_name(elf, &ehdr, &shdr, ".dynamic", NULL);
>>> + if (!scn)
>>> + goto exit; // false
>>
>> It should goto a new tag (e.g. exit_failed) so can invoke elf_end(elf) and
>> close(fd).
>>
>>> +
>>> + data = (Elf_Data *) elf_getdata(scn, NULL);
>>> + if (!data || !data->d_buf)
>>> + goto exit; // false
>>
>> Ditto.
>>
>>> +
>>> + // check DT_FLAGS_1
>>> + for (int i = 0; i < n_entries; i++) {
>>> + entry = ((GElf_Dyn *) data->d_buf) + i;
>>> + if (entry->d_tag == DT_FLAGS_1) {
>>> + if ((entry->d_un.d_val & DF_1_PIE) != 0) {
>>> + is_pie = true;
>>> + break;
>>> + }
>>> + }
>>> + } // end for
>>> + }
>>> +
>>
>> Put 'exit_failed' tag at here.
>>
>> With the changes:
>>
>> Reviewed-by: Leo Yan <leo.yan@arm.com>
>>
>>> + elf_end(elf);
>>> + close(fd);
>>> +exit:
>>> + return is_pie;
>>> +}
>>> +
>
> Hi Leo,
>
> It's been a long time since I've seen goto's used as a rule rather than
> an exception. I see it introduced the problem you've mentioned.
I am not sure if I understand this. In the Linux code, it is quite common to
use goto to handle failure cases (for releasing resources).
> Is it ok to simply update the cover letter (patch 0), and this one (patch 1)
> as V5? If I don't hear from you, I'll resubmit all.
I assume you also need to address patch 03? It is good to send the all patches
in one go, this would be easier for maintainers to pick up the whole series
(e.g. using b4).
Thanks,
Leo
^ permalink raw reply [flat|nested] 13+ messages in thread