| Message ID | 1337629910-30302-5-git-send-email-jim@meyering.net |
|---|---|
| State | New |
| Headers | show |
On 21 May 2012 20:51, Jim Meyering <jim@meyering.net> wrote: > From: Jim Meyering <meyering@redhat.com> > > > Signed-off-by: Jim Meyering <meyering@redhat.com> > --- > tcg/tcg.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tcg/tcg.c b/tcg/tcg.c > index ab589c7..350fdad 100644 > --- a/tcg/tcg.c > +++ b/tcg/tcg.c > @@ -2293,7 +2293,7 @@ void __jit_debug_register_code(void) > > /* Must statically initialize the version, because GDB may check > the version before we can set it. */ > -struct jit_descriptor __jit_debug_descriptor = { 1, 0, 0, 0 }; > +static struct jit_descriptor __jit_debug_descriptor = { 1, 0, 0, 0 }; > > /* End GDB interface. */ Nak. This symbol is global so that gdb can find it by fishing around in the executable's symbol table. -- PMM
Peter Maydell wrote: > On 21 May 2012 20:51, Jim Meyering <jim@meyering.net> wrote: >> From: Jim Meyering <meyering@redhat.com> >> >> >> Signed-off-by: Jim Meyering <meyering@redhat.com> >> --- >> tcg/tcg.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/tcg/tcg.c b/tcg/tcg.c >> index ab589c7..350fdad 100644 >> --- a/tcg/tcg.c >> +++ b/tcg/tcg.c >> @@ -2293,7 +2293,7 @@ void __jit_debug_register_code(void) >> >> /* Must statically initialize the version, because GDB may check >> the version before we can set it. */ >> -struct jit_descriptor __jit_debug_descriptor = { 1, 0, 0, 0 }; >> +static struct jit_descriptor __jit_debug_descriptor = { 1, 0, 0, 0 }; >> >> /* End GDB interface. */ > > Nak. This symbol is global so that gdb can find it by fishing around > in the executable's symbol table. Thanks for the quick feedback. How does the scope of the symbol affect whether gdb can find it? With it declared static, it seems to be no less visible than before: $ gdb --eval 'p __jit_debug_descriptor' x86_64-softmmu/qemu-system-x86_64 Reading symbols from /h/j/w/co/qemu/x86_64-softmmu/qemu-system-x86_64...done. $1 = { version = 1, action_flag = 0, relevant_entry = 0x0, first_entry = 0x0 } If declaring this variable "static" is not appropriate, then the comment saying that static initialization is desired should be changed.
On 21 May 2012 21:10, Jim Meyering <jim@meyering.net> wrote: > Peter Maydell wrote: >> On 21 May 2012 20:51, Jim Meyering <jim@meyering.net> wrote: >>> From: Jim Meyering <meyering@redhat.com> >>> >>> >>> Signed-off-by: Jim Meyering <meyering@redhat.com> >>> --- >>> tcg/tcg.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/tcg/tcg.c b/tcg/tcg.c >>> index ab589c7..350fdad 100644 >>> --- a/tcg/tcg.c >>> +++ b/tcg/tcg.c >>> @@ -2293,7 +2293,7 @@ void __jit_debug_register_code(void) >>> >>> /* Must statically initialize the version, because GDB may check >>> the version before we can set it. */ >>> -struct jit_descriptor __jit_debug_descriptor = { 1, 0, 0, 0 }; >>> +static struct jit_descriptor __jit_debug_descriptor = { 1, 0, 0, 0 }; >>> >>> /* End GDB interface. */ >> >> Nak. This symbol is global so that gdb can find it by fishing around >> in the executable's symbol table. > > Thanks for the quick feedback. > > How does the scope of the symbol affect whether gdb can find it? If you mark it 'static' the compiler can throw it away or completely rearrange it if it's feeling clever enough, I think. Anyway, we're following a prescribed interface here: http://sourceware.org/gdb/onlinedocs/gdb/Declarations.html and I don't think we should deviate from it. As the comment says, "THE FOLLOWING MUST MATCH GDB DOCS.". > If declaring this variable "static" is not appropriate, > then the comment saying that static initialization is desired > should be changed. The comment means "statically initialize this variable rather than doing it dynamically in some function at startup". -- PMM
Peter Maydell wrote: > On 21 May 2012 21:10, Jim Meyering <jim@meyering.net> wrote: >> Peter Maydell wrote: >>> On 21 May 2012 20:51, Jim Meyering <jim@meyering.net> wrote: >>>> From: Jim Meyering <meyering@redhat.com> >>>> >>>> >>>> Signed-off-by: Jim Meyering <meyering@redhat.com> >>>> --- >>>> tcg/tcg.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/tcg/tcg.c b/tcg/tcg.c >>>> index ab589c7..350fdad 100644 >>>> --- a/tcg/tcg.c >>>> +++ b/tcg/tcg.c >>>> @@ -2293,7 +2293,7 @@ void __jit_debug_register_code(void) >>>> >>>> /* Must statically initialize the version, because GDB may check >>>> the version before we can set it. */ >>>> -struct jit_descriptor __jit_debug_descriptor = { 1, 0, 0, 0 }; >>>> +static struct jit_descriptor __jit_debug_descriptor = { 1, 0, 0, 0 }; >>>> >>>> /* End GDB interface. */ >>> >>> Nak. This symbol is global so that gdb can find it by fishing around >>> in the executable's symbol table. >> >> Thanks for the quick feedback. >> >> How does the scope of the symbol affect whether gdb can find it? > > If you mark it 'static' the compiler can throw it away or completely > rearrange it if it's feeling clever enough, I think. > > Anyway, we're following a prescribed interface here: > http://sourceware.org/gdb/onlinedocs/gdb/Declarations.html > > and I don't think we should deviate from it. As the comment says, > "THE FOLLOWING MUST MATCH GDB DOCS.". > >> If declaring this variable "static" is not appropriate, >> then the comment saying that static initialization is desired >> should be changed. > > The comment means "statically initialize this variable rather than > doing it dynamically in some function at startup". Thanks. I've clarified the comments and posted a V2.
diff --git a/tcg/tcg.c b/tcg/tcg.c index ab589c7..350fdad 100644 --- a/tcg/tcg.c +++ b/tcg/tcg.c @@ -2293,7 +2293,7 @@ void __jit_debug_register_code(void) /* Must statically initialize the version, because GDB may check the version before we can set it. */ -struct jit_descriptor __jit_debug_descriptor = { 1, 0, 0, 0 }; +static struct jit_descriptor __jit_debug_descriptor = { 1, 0, 0, 0 }; /* End GDB interface. */