Patchwork linux-user: Correct DLINFO_ITEMS

login
register
mail settings
Submitter James Hogan
Date March 25, 2014, 9:47 p.m.
Message ID <1395784048-28123-1-git-send-email-james.hogan@imgtec.com>
Download mbox | patch
Permalink /patch/333728/
State New
Headers show

Comments

James Hogan - March 25, 2014, 9:47 p.m.
Commit a07c67dfccb1 (Implement AT_CLKTCK.) back in March 2008 added a
new auxvec entry but didn't increment DLINFO_ITEMS, so it's been out of
sync ever since.

Bump it up to 14 so that it matches the number of NEW_AUX_ENT's that
need to be counted in create_elf_tables().

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Riku Voipio <riku.voipio@iki.fi>
Cc: Paul Brook <paul@codesourcery.com>
---
 linux-user/elfload.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Peter Maydell - March 25, 2014, 10:20 p.m.
On 25 March 2014 21:47, James Hogan <james.hogan@imgtec.com> wrote:
> Commit a07c67dfccb1 (Implement AT_CLKTCK.) back in March 2008 added a
> new auxvec entry but didn't increment DLINFO_ITEMS, so it's been out of
> sync ever since.
>
> Bump it up to 14 so that it matches the number of NEW_AUX_ENT's that
> need to be counted in create_elf_tables().

This code could clearly use at least an assert that we've not written more
entries than we should, or ideally restructuring somehow so that we don't
have to set DLINFO_ITEMS by hand in the first place...

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
for the immediate fix, though.

thanks
-- PMM
James Hogan - March 25, 2014, 11:40 p.m.
On Tuesday 25 March 2014 22:20:04 Peter Maydell wrote:
> On 25 March 2014 21:47, James Hogan <james.hogan@imgtec.com> wrote:
> > Commit a07c67dfccb1 (Implement AT_CLKTCK.) back in March 2008 added a
> > new auxvec entry but didn't increment DLINFO_ITEMS, so it's been out of
> > sync ever since.
> > 
> > Bump it up to 14 so that it matches the number of NEW_AUX_ENT's that
> > need to be counted in create_elf_tables().
> 
> This code could clearly use at least an assert that we've not written more
> entries than we should, or ideally restructuring somehow so that we don't
> have to set DLINFO_ITEMS by hand in the first place...

Good idea. I've just submitted a patch to add an assert that the allocated 
stack for auxvec/envp/argv matches the amount used.

I've already re-factored a bunch of this code to handle stacks which grow 
upwards (as used for HPPA and Meta arches), which would make it even more 
awkward to avoid DLINFO_ITEMS since it still builds the auxvec, envp and argv 
downwards within a new stack frame (grown upwards), so it also needs to know 
how much stack frame to allocate in advance to be sure it doesn't clobber 
other stuff lower on the stack.

> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> for the immediate fix, though.

Thanks for reviewing,

Cheers
James
Peter Maydell - March 25, 2014, 11:51 p.m.
On 25 March 2014 23:40, James Hogan <james.hogan@imgtec.com> wrote:
> I've already re-factored a bunch of this code to handle stacks which grow
> upwards (as used for HPPA and Meta arches)

...we don't support either of those currently, do you plan to submit
something?

thanks
-- PMM
James Hogan - March 26, 2014, 12:27 a.m.
On Tuesday 25 March 2014 23:51:39 Peter Maydell wrote:
> On 25 March 2014 23:40, James Hogan <james.hogan@imgtec.com> wrote:
> > I've already re-factored a bunch of this code to handle stacks which grow
> > upwards (as used for HPPA and Meta arches)
> 
> ...we don't support either of those currently, do you plan to submit
> something?

I'm not sure yet. I have the okay to mainline our Meta QEMU fork[1], but it's 
fallen a little behind (v1.3.1 - and in my experiencing updating an out of 
tree QEMU fork is painful), so I've just been taking another look in my own 
time to see what it would take.

Cheers
James

[1] https://github.com/img-meta/qemu/commit/9355e4b1929c7f05a103dc0e76fe44b54d12b2a7
Riku Voipio - March 26, 2014, 12:08 p.m.
Hi,

On Tue, Mar 25, 2014 at 10:20:04PM +0000, Peter Maydell wrote:
> On 25 March 2014 21:47, James Hogan <james.hogan@imgtec.com> wrote:
> > Commit a07c67dfccb1 (Implement AT_CLKTCK.) back in March 2008 added a
> > new auxvec entry but didn't increment DLINFO_ITEMS, so it's been out of
> > sync ever since.
> >
> > Bump it up to 14 so that it matches the number of NEW_AUX_ENT's that
> > need to be counted in create_elf_tables().
> 
> This code could clearly use at least an assert that we've not written more
> entries than we should, or ideally restructuring somehow so that we don't
> have to set DLINFO_ITEMS by hand in the first place...
 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> for the immediate fix, though.

I've applied this and the other paches from you to my tree:

https://git.linaro.org/people/riku.voipio/qemu.git/shortlog/refs/heads/linux-user-for-upstream

I think others are ok to be applied post-2.0 release, but I'll
send this for 2.0

Riku

Riku

Patch

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 99a2c58..d2380b6 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1076,7 +1076,7 @@  struct exec
 #define TARGET_ELF_PAGESTART(_v) ((_v) & ~(unsigned long)(TARGET_ELF_EXEC_PAGESIZE-1))
 #define TARGET_ELF_PAGEOFFSET(_v) ((_v) & (TARGET_ELF_EXEC_PAGESIZE-1))
 
-#define DLINFO_ITEMS 13
+#define DLINFO_ITEMS 14
 
 static inline void memcpy_fromfs(void * to, const void * from, unsigned long n)
 {