<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 02/27/2014 12:22 PM, Petr Machata
wrote:<br>
</div>
<blockquote cite="mid:m2a9dc7qbi.fsf@redhat.com" type="cite">
<pre wrap="">Thanks, this will save me quite a bit of work figuring out how the new
ABI works.
"Thierry@vnet" <a class="moz-txt-link-rfc2396E" href="mailto:thierry@linux.vnet.ibm.com"><thierry@linux.vnet.ibm.com></a> writes:
</pre>
<blockquote type="cite">
<pre wrap="">diff --git a/ltrace-0.7.3.orig/sysdeps/linux-gnu/ppc/arch.h b/ltrace-0.7.3/sysdeps/linux-gnu/ppc/arch.h
index fb8768a..edabe2e 100644
--- a/ltrace-0.7.3.orig/sysdeps/linux-gnu/ppc/arch.h
+++ b/ltrace-0.7.3/sysdeps/linux-gnu/ppc/arch.h
@@ -27,6 +27,7 @@
#define BREAKPOINT_VALUE { 0x7f, 0xe0, 0x00, 0x08 }
#define BREAKPOINT_LENGTH 4
#define DECR_PC_AFTER_BREAK 0
+#define ARCH_ENDIAN_BIG
#define LT_ELFCLASS ELFCLASS32
#define LT_ELF_MACHINE EM_PPC
@@ -35,6 +36,12 @@
#define LT_ELFCLASS2 ELFCLASS64
#define LT_ELF_MACHINE2 EM_PPC64
#define ARCH_SUPPORTS_OPD
+#ifdef __LITTLE_ENDIAN__
+#undef BREAKPOINT_VALUE
+#define BREAKPOINT_VALUE { 0x08, 0x00, 0xe0, 0x7f }
+#define ARCH_ENDIAN_LITTLE
+#undef ARCH_ENDIAN_BIG
+#endif
</pre>
</blockquote>
<pre wrap="">
Please don't #undef, instead conditionally #define one or the other.</pre>
</blockquote>
Ok - thought it was safer to avoid special case. But definitely
setting the BREAKPOINT_VALUE upon arch is easy.
<blockquote cite="mid:m2a9dc7qbi.fsf@redhat.com" type="cite">
<blockquote type="cite">
<pre wrap="">diff --git a/ltrace.0.7.3-v1/ltrace-elf.c b/ltrace-0.7.3/ltrace-elf.c
index c571d2a..4b8b0bc 100644
--- a/ltrace.0.7.3-v1/ltrace-elf.c
+++ b/ltrace-0.7.3/ltrace-elf.c
@@ -714,6 +714,10 @@ populate_this_symtab(struct Process *proc, const char *filename,
continue;
}
+#if defined(__powerpc64__) && _CALL_ELF == 2
+ naddr += PPC64_LOCAL_ENTRY_OFFSET (sym.st_other);
+#endif
+
</pre>
</blockquote>
<pre wrap="">
Where does _CALL_ELF come from? What is PPC64_LOCAL_ENTRY_OFFSET? In
any case, this should be folded into arch_translate_address in PPC
backend.</pre>
</blockquote>
The PPC64_LOCAL_ENTRY_OFFSE() is a macro in elf.h<br>
* PowerPC64 specific values for the Elf64_Sym st_other field. */<br>
#define STO_PPC64_LOCAL_BIT 5<br>
#define STO_PPC64_LOCAL_MASK (7 << STO_PPC64_LOCAL_BIT)<br>
#define PPC64_LOCAL_ENTRY_OFFSET(other) \<br>
(((1 << (((other) & STO_PPC64_LOCAL_MASK) >>
STO_PPC64_LOCAL_BIT)) >> 2) << 2)<br>
<br>
That entry in ltrace-elf.c is definitely not good and needs to be
placed in */*/ppc/plt.c , but currently we haven't find a proper
place to have it - my though was of course to use the
arch_translate_addres() but I have 2 issues there<br>
- we sometime are in the case we don't call it<br>
- As I don't know the context (saying it is a symbol) , I have
to check the address is in the good range - that make sense. <br>
I will work on it . <br>
<blockquote cite="mid:m2a9dc7qbi.fsf@redhat.com" type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">+#if _CALL_ELF == 2
+#undef ARCH_SUPPORTS_OPD
+#endif
</pre>
</blockquote>
<pre wrap="">
Again, either #define or don't #define, but don't #undef.
</pre>
</blockquote>
yes<br>
<blockquote cite="mid:m2a9dc7qbi.fsf@redhat.com" type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">+enum homogeneous_type {
+ NOINIT = 0,
+ HETEROGENEOUS,
+ HOMOGENEOUS,
+ HOMOGENEOUS_NESTED_FLOAT,
+};
+
+struct struct_attributes {
+ struct arg_type_info *info;
+ enum arg_type type;
+ size_t nb_elements;
+ enum homogeneous_type homogeneous;
+};
+
+
</pre>
</blockquote>
<pre wrap="">
Why can't you use type_get_hfa_type? Can we extend it so that it does
what you need?
</pre>
</blockquote>
That code written by Guy (taken from other bits) is definitely a
first drop and needs enhancement.<br>
I (We) will send proposal for enhancements.
<blockquote cite="mid:m2a9dc7qbi.fsf@redhat.com" type="cite">
<blockquote type="cite">
<pre wrap="">+ if (ret_info->type == ARGTYPE_STRUCT) {
+ get_struct_attribut(ret_info, proc, &ret_size, &struct_attr);
+
+ if (((struct_attr.homogeneous == HOMOGENEOUS_NESTED_FLOAT) &&
+ (struct_attr.nb_elements > 8)) ||
+ (((struct_attr.homogeneous == HOMOGENEOUS) ||
+ (struct_attr.homogeneous == HETEROGENEOUS)) &&
+ (ret_size > 16)))
</pre>
</blockquote>
<pre wrap="">
Please don't leave && and || hanging on the end of a line. Also indent
properly (what follows after the first && should be two characters
ahead). Personally I'd write it thus:
+ if ((struct_attr.homogeneous == HOMOGENEOUS_NESTED_FLOAT
+ && struct_attr.nb_elements > 8)
+ || ((struct_attr.homogeneous == HOMOGENEOUS
+ || struct_attr.homogeneous == HETEROGENEOUS)
+ && ret_size > 16))
There are more instances of this problem, I won't cite them all.</pre>
</blockquote>
Agree on that - I will transmit ... we are learning.
<blockquote cite="mid:m2a9dc7qbi.fsf@redhat.com" type="cite">
<blockquote type="cite">
<pre wrap="">+struct arg_type_info *
+arch_type_get_fp_equivalent(struct arg_type_info *info, struct Process *proc)
+{
</pre>
</blockquote>
<pre wrap="">
What is this good for?
</pre>
<blockquote type="cite">
<pre wrap=""> unsigned char *buf = value_reserve(valuep, slots * width);
if (buf == NULL)
return -1;
- struct arg_type_info *long_info = type_get_simple(ARGTYPE_LONG);
+ struct arg_type_info *long_info = arch_type_get_simple(ARGTYPE_LONG,proc);
</pre>
</blockquote>
<pre wrap="">
I see you undid this change in a follow-up patch. It would be better if
you could fold the fixes into this one, so that each patch can be
reviewed and tested separately.</pre>
</blockquote>
Yes my mistake while reading the patch ...bit tired when I double
checked it . will provide the clean patch
<blockquote cite="mid:m2a9dc7qbi.fsf@redhat.com" type="cite">
<blockquote type="cite">
<pre wrap="">+#if _CALL_ELF != 2
#define PPC64_PLT_STUB_SIZE 8 //xxx
+#else
+#define PPC64_PLT_STUB_SIZE 4 //xxx
+#endif
</pre>
</blockquote>
<pre wrap="">
Let's drop both xxx's. Cleary that's what it is at this moment.</pre>
</blockquote>
ok<br>
<blockquote cite="mid:m2a9dc7qbi.fsf@redhat.com" type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap=""> arch_translate_address(struct ltelf *lte,
arch_addr_t addr, arch_addr_t *ret)
{
- if (lte->ehdr.e_machine == EM_PPC64) {
+ if (lte->ehdr.e_machine == EM_PPC64
+ && (lte->ehdr.e_flags & 3) != 2 ) {
</pre>
</blockquote>
<pre wrap="">
Do these numbers have symbolic names by any chance?</pre>
</blockquote>
Not as far as I know ( not in all *elf.h) on system - I will check
the asm bits since its coming from there.<br>
<blockquote cite="mid:m2a9dc7qbi.fsf@redhat.com" type="cite">
<blockquote type="cite">
<pre wrap=""> /* XXX The double cast should be removed when
* arch_addr_t becomes integral type. */
GElf_Xword offset
@@ -433,6 +444,7 @@ int
arch_elf_init(struct ltelf *lte, struct library *lib)
{
if (lte->ehdr.e_machine == EM_PPC64
+ && (lte->ehdr.e_flags & 3) != 2
</pre>
</blockquote>
<pre wrap="">
And again here... clearly we need to name this and refer back to it
instead of testing the same thing again and again. It seems as if the
following is necessary:
diff --git a/sysdeps/linux-gnu/ppc/arch.h b/sysdeps/linux-gnu/ppc/arch.h
index bf9b5dc..664a670 100644
--- a/sysdeps/linux-gnu/ppc/arch.h
+++ b/sysdeps/linux-gnu/ppc/arch.h
@@ -56,7 +56,8 @@ struct arch_ltelf_data {
Elf_Data *opd_data;
GElf_Addr opd_base;
GElf_Xword opd_size;
- int secure_plt;
+ bool secure_plt : 1;
+ bool elfv2_abi : 1;
Elf_Data *reladyn;
size_t reladyn_count;
@@ -65,7 +66,8 @@ struct arch_ltelf_data {
#define ARCH_HAVE_LIBRARY_DATA
struct arch_library_data {
GElf_Addr pltgot_addr;
- int bss_plt_prelinked;
+ bool bss_plt_prelinked : 1;
+ bool elfv2_abi : 1;
};
enum ppc64_plt_type {
(Likely just one of these will be necessary.)
This should be initialized in arch_elf_init and later used in lieu of
all the #ifdef stuff. Since the ELFv2 is purely a userspace thing, this
would allow the theoretical case of tracing ELFv2 process with ELFv1
ltrace. More importantly, it would expose all the code to the compiler,
which prevents bitrot somewhat.</pre>
</blockquote>
Agree on using that implementation. Thanks for pointing out.<br>
<blockquote cite="mid:m2a9dc7qbi.fsf@redhat.com" type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">@@ -862,9 +872,13 @@ ppc_plt_bp_continue(struct breakpoint *bp, struct Process *proc)
(struct process_stopping_handler *);
case PPC_DEFAULT:
+#if _CALL_ELF != 2
assert(proc->e_machine == EM_PPC);
assert(bp->libsym != NULL);
assert(bp->libsym->lib->arch.bss_plt_prelinked == 0);
+#else
+ /* ABIv2 uses plt and may set the flag */
+#endif
</pre>
</blockquote>
<pre wrap="">
PPC_DEFAULT is for non-PLT or PPC32 symbols. Is the PLT scheme similar
on ELFv2 to what ELFv1 does, or is it "plain old PLT" like other arches
do? I.e. does the comment on top of plt.c apply?
If it's plain old PLT, then it seems we shouldn't fall through to
PPC_PLT_UNRESOLVED. If it's similar to what PPC64 ELFv1 does, then
PPC_DEFAULT is not the proper type for this breakpoint.</pre>
</blockquote>
Since .opd disapeared, PLT is more like what we have on other
systems - and <br>
<blockquote>
<pre wrap="">In ELFv2, the PLT
is an array of plain (function) pointers, with the first pointing
to _dl_runtime_resolve and the second holding the map address.
</pre>
</blockquote>
<blockquote>
<pre wrap="">
</pre>
</blockquote>
<pre wrap="">As result of your comment, I realized that the new test trace-irelative.exp is failing twice since the xyz entry is not added to the PLT after arch_elf_init() and such not initialized as a breakpoint. I need to review the arch_elf_init() code then.
We found also few (6) problems with the new hfa tests and this is under analysis.
Great thanks for your help.
Thierry
</pre>
<blockquote cite="mid:m2a9dc7qbi.fsf@redhat.com" type="cite">
</blockquote>
<br>
<br>
<pre class="moz-signature" cols="72">--
Thierry on vnet.ibm</pre>
</body>
</html>