Add vmcoreinfo ELF note if vmcoreinfo device is ready.

To help the python script, add a little global vmcoreinfo_gdb
structure, that is populated with vmcoreinfo_gdb_update().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 scripts/dump-guest-memory.py | 40 ++++++++++++++++++++++++++++++++++++++++
 hw/acpi/vmcoreinfo.c         |  3 +++
 2 files changed, 43 insertions(+)

diff --git a/scripts/dump-guest-memory.py b/scripts/dump-guest-memory.py
index f7c6635f15..2dd2ed6983 100644
--- a/scripts/dump-guest-memory.py
+++ b/scripts/dump-guest-memory.py
@@ -14,6 +14,7 @@ the COPYING file in the top-level directory.
 """
 
 import ctypes
+import struct
 
 UINTPTR_T = gdb.lookup_type("uintptr_t")
 
@@ -120,6 +121,20 @@ class ELF(object):
         self.segments[0].p_filesz += ctypes.sizeof(note)
         self.segments[0].p_memsz += ctypes.sizeof(note)
 
+
+    def add_vmcoreinfo_note(self, vmcoreinfo):
+        """Adds a vmcoreinfo note to the ELF dump."""
+        chead = type(get_arch_note(self.endianness, 0, 0))
+        header = chead.from_buffer_copy(vmcoreinfo[0:ctypes.sizeof(chead)])
+        note = get_arch_note(self.endianness,
+                             header.n_namesz - 1, header.n_descsz)
+        ctypes.memmove(ctypes.pointer(note), vmcoreinfo, ctypes.sizeof(note))
+        header_size = ctypes.sizeof(note) - header.n_descsz
+
+        self.notes.append(note)
+        self.segments[0].p_filesz += ctypes.sizeof(note)
+        self.segments[0].p_memsz += ctypes.sizeof(note)
+
     def add_segment(self, p_type, p_paddr, p_size):
         """Adds a segment to the elf."""
 
@@ -505,6 +520,30 @@ shape and this command should mostly work."""
                 cur += chunk_size
                 left -= chunk_size
 
+    def phys_memory_read(self, addr, size):
+        qemu_core = gdb.inferiors()[0]
+        for block in self.guest_phys_blocks:
+            if block["target_start"] <= addr < block["target_end"]:
+                haddr = block["host_addr"] + (addr - block["target_start"])
+                return qemu_core.read_memory(haddr, size)
+
+    def add_vmcoreinfo(self):
+        if not gdb.parse_and_eval("vmcoreinfo_gdb_helper"):
+            return
+
+        addr = gdb.parse_and_eval("vmcoreinfo_gdb_helper.vmcoreinfo_addr_le")
+        addr = bytes([addr[i] for i in range(4)])
+        addr = struct.unpack("<I", addr)[0]
+
+        mem = self.phys_memory_read(addr, 16)
+        (version, addr, size) = struct.unpack("<IQI", mem)
+        if version != 0:
+            return
+
+        vmcoreinfo = self.phys_memory_read(addr, size)
+        if vmcoreinfo:
+            self.elf.add_vmcoreinfo_note(vmcoreinfo.tobytes())
+
     def invoke(self, args, from_tty):
         """Handles command invocation from gdb."""
 
@@ -518,6 +557,7 @@ shape and this command should mostly work."""
 
         self.elf = ELF(argv[1])
         self.guest_phys_blocks = get_guest_phys_blocks()
+        self.add_vmcoreinfo()
 
         with open(argv[0], "wb") as vmcore:
             self.dump_init(vmcore)
diff --git a/hw/acpi/vmcoreinfo.c b/hw/acpi/vmcoreinfo.c
index 0ea41de8d9..b6bcb47506 100644
--- a/hw/acpi/vmcoreinfo.c
+++ b/hw/acpi/vmcoreinfo.c
@@ -163,6 +163,8 @@ static void vmcoreinfo_handle_reset(void *opaque)
     memset(vis->vmcoreinfo_addr_le, 0, ARRAY_SIZE(vis->vmcoreinfo_addr_le));
 }
 
+static VMCoreInfoState *vmcoreinfo_gdb_helper;
+
 static void vmcoreinfo_realize(DeviceState *dev, Error **errp)
 {
     if (!bios_linker_loader_can_write_pointer()) {
@@ -181,6 +183,7 @@ static void vmcoreinfo_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    vmcoreinfo_gdb_helper = VMCOREINFO(dev);
     qemu_register_reset(vmcoreinfo_handle_reset, dev);
 }
 
-- 
2.13.1.395.gf7b71de06

On 07/06/17 12:16, Marc-André Lureau wrote:
> Add vmcoreinfo ELF note if vmcoreinfo device is ready.
> 
> To help the python script, add a little global vmcoreinfo_gdb
> structure, that is populated with vmcoreinfo_gdb_update().
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  scripts/dump-guest-memory.py | 40 ++++++++++++++++++++++++++++++++++++++++
>  hw/acpi/vmcoreinfo.c         |  3 +++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/scripts/dump-guest-memory.py b/scripts/dump-guest-memory.py
> index f7c6635f15..2dd2ed6983 100644
> --- a/scripts/dump-guest-memory.py
> +++ b/scripts/dump-guest-memory.py
> @@ -14,6 +14,7 @@ the COPYING file in the top-level directory.
>  """
>  
>  import ctypes
> +import struct
>  
>  UINTPTR_T = gdb.lookup_type("uintptr_t")
>  
> @@ -120,6 +121,20 @@ class ELF(object):
>          self.segments[0].p_filesz += ctypes.sizeof(note)
>          self.segments[0].p_memsz += ctypes.sizeof(note)
>  
> +
> +    def add_vmcoreinfo_note(self, vmcoreinfo):
> +        """Adds a vmcoreinfo note to the ELF dump."""
> +        chead = type(get_arch_note(self.endianness, 0, 0))
> +        header = chead.from_buffer_copy(vmcoreinfo[0:ctypes.sizeof(chead)])

Maybe it's obvious to others, but I would have been helped a lot if a
comment had explained that you are creating a fake note (with 0 desc
size and 0 name size) to figure out the size of the note header. And
then you copy that many bytes out of the vmcoreinfo ELF note.

> +        note = get_arch_note(self.endianness,
> +                             header.n_namesz - 1, header.n_descsz)

Why the -1?

... I think I'm giving up here for this method. My python is weak and I
can't follow this too well. Please add some comments.

More comments below:

> +        ctypes.memmove(ctypes.pointer(note), vmcoreinfo, ctypes.sizeof(note))
> +        header_size = ctypes.sizeof(note) - header.n_descsz
> +
> +        self.notes.append(note)
> +        self.segments[0].p_filesz += ctypes.sizeof(note)
> +        self.segments[0].p_memsz += ctypes.sizeof(note)
> +
>      def add_segment(self, p_type, p_paddr, p_size):
>          """Adds a segment to the elf."""
>  
> @@ -505,6 +520,30 @@ shape and this command should mostly work."""
>                  cur += chunk_size
>                  left -= chunk_size
>  
> +    def phys_memory_read(self, addr, size):
> +        qemu_core = gdb.inferiors()[0]
> +        for block in self.guest_phys_blocks:
> +            if block["target_start"] <= addr < block["target_end"]:

Although I don't expect a single read to straddle phys-blocks, I would
prefer if you checked (addr + size) -- and not just addr -- against
block["target_end"].

> +                haddr = block["host_addr"] + (addr - block["target_start"])
> +                return qemu_core.read_memory(haddr, size)
> +
> +    def add_vmcoreinfo(self):
> +        if not gdb.parse_and_eval("vmcoreinfo_gdb_helper"):
> +            return
> +
> +        addr = gdb.parse_and_eval("vmcoreinfo_gdb_helper.vmcoreinfo_addr_le")
> +        addr = bytes([addr[i] for i in range(4)])
> +        addr = struct.unpack("<I", addr)[0]
> +
> +        mem = self.phys_memory_read(addr, 16)
> +        (version, addr, size) = struct.unpack("<IQI", mem)
> +        if version != 0:
> +            return
> +
> +        vmcoreinfo = self.phys_memory_read(addr, size)
> +        if vmcoreinfo:
> +            self.elf.add_vmcoreinfo_note(vmcoreinfo.tobytes())
> +
>      def invoke(self, args, from_tty):
>          """Handles command invocation from gdb."""
>  
> @@ -518,6 +557,7 @@ shape and this command should mostly work."""
>  
>          self.elf = ELF(argv[1])
>          self.guest_phys_blocks = get_guest_phys_blocks()
> +        self.add_vmcoreinfo()
>  
>          with open(argv[0], "wb") as vmcore:
>              self.dump_init(vmcore)
> diff --git a/hw/acpi/vmcoreinfo.c b/hw/acpi/vmcoreinfo.c
> index 0ea41de8d9..b6bcb47506 100644
> --- a/hw/acpi/vmcoreinfo.c
> +++ b/hw/acpi/vmcoreinfo.c
> @@ -163,6 +163,8 @@ static void vmcoreinfo_handle_reset(void *opaque)
>      memset(vis->vmcoreinfo_addr_le, 0, ARRAY_SIZE(vis->vmcoreinfo_addr_le));
>  }
>  
> +static VMCoreInfoState *vmcoreinfo_gdb_helper;
> +
>  static void vmcoreinfo_realize(DeviceState *dev, Error **errp)
>  {
>      if (!bios_linker_loader_can_write_pointer()) {
> @@ -181,6 +183,7 @@ static void vmcoreinfo_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> +    vmcoreinfo_gdb_helper = VMCOREINFO(dev);
>      qemu_register_reset(vmcoreinfo_handle_reset, dev);
>  }
>  
> 

I guess we don't build QEMU with link-time optimization at the moment.

With link-time optimization, I think gcc might reasonably optimize away
the assignment to "vmcoreinfo_gdb_helper", and "vmcoreinfo_gdb_helper"
itself. This is why I suggested "volatile":

static VMCoreInfoState * volatile vmcoreinfo_gdb_helper;

Do you think volatile is only superfluous, or do you actively dislike it
for some reason?

Thanks,
Laszlo

Hi

----- Original Message -----
> On 07/06/17 12:16, Marc-André Lureau wrote:
> > Add vmcoreinfo ELF note if vmcoreinfo device is ready.
> > 
> > To help the python script, add a little global vmcoreinfo_gdb
> > structure, that is populated with vmcoreinfo_gdb_update().
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  scripts/dump-guest-memory.py | 40 ++++++++++++++++++++++++++++++++++++++++
> >  hw/acpi/vmcoreinfo.c         |  3 +++
> >  2 files changed, 43 insertions(+)
> > 
> > diff --git a/scripts/dump-guest-memory.py b/scripts/dump-guest-memory.py
> > index f7c6635f15..2dd2ed6983 100644
> > --- a/scripts/dump-guest-memory.py
> > +++ b/scripts/dump-guest-memory.py
> > @@ -14,6 +14,7 @@ the COPYING file in the top-level directory.
> >  """
> >  
> >  import ctypes
> > +import struct
> >  
> >  UINTPTR_T = gdb.lookup_type("uintptr_t")
> >  
> > @@ -120,6 +121,20 @@ class ELF(object):
> >          self.segments[0].p_filesz += ctypes.sizeof(note)
> >          self.segments[0].p_memsz += ctypes.sizeof(note)
> >  
> > +
> > +    def add_vmcoreinfo_note(self, vmcoreinfo):
> > +        """Adds a vmcoreinfo note to the ELF dump."""
> > +        chead = type(get_arch_note(self.endianness, 0, 0))
> > +        header =
> > chead.from_buffer_copy(vmcoreinfo[0:ctypes.sizeof(chead)])
> 
> Maybe it's obvious to others, but I would have been helped a lot if a
> comment had explained that you are creating a fake note (with 0 desc
> size and 0 name size) to figure out the size of the note header. And
> then you copy that many bytes out of the vmcoreinfo ELF note.
> 

ok

> > +        note = get_arch_note(self.endianness,
> > +                             header.n_namesz - 1, header.n_descsz)
> 
> Why the -1?

because get_arch_note() adds + 1 (supposedly for the ending \0, see also add_note())

> 
> ... I think I'm giving up here for this method. My python is weak and I
> can't follow this too well. Please add some comments.

I simplified a bit the code now, I was also discovering some ctype usage.

> More comments below:
> 
> > +        ctypes.memmove(ctypes.pointer(note), vmcoreinfo,
> > ctypes.sizeof(note))
> > +        header_size = ctypes.sizeof(note) - header.n_descsz
> > +
> > +        self.notes.append(note)
> > +        self.segments[0].p_filesz += ctypes.sizeof(note)
> > +        self.segments[0].p_memsz += ctypes.sizeof(note)
> > +
> >      def add_segment(self, p_type, p_paddr, p_size):
> >          """Adds a segment to the elf."""
> >  
> > @@ -505,6 +520,30 @@ shape and this command should mostly work."""
> >                  cur += chunk_size
> >                  left -= chunk_size
> >  
> > +    def phys_memory_read(self, addr, size):
> > +        qemu_core = gdb.inferiors()[0]
> > +        for block in self.guest_phys_blocks:
> > +            if block["target_start"] <= addr < block["target_end"]:
> 
> Although I don't expect a single read to straddle phys-blocks, I would
> prefer if you checked (addr + size) -- and not just addr -- against
> block["target_end"].

done

> 
> > +                haddr = block["host_addr"] + (addr -
> > block["target_start"])
> > +                return qemu_core.read_memory(haddr, size)
> > +
> > +    def add_vmcoreinfo(self):
> > +        if not gdb.parse_and_eval("vmcoreinfo_gdb_helper"):
> > +            return
> > +
> > +        addr =
> > gdb.parse_and_eval("vmcoreinfo_gdb_helper.vmcoreinfo_addr_le")
> > +        addr = bytes([addr[i] for i in range(4)])
> > +        addr = struct.unpack("<I", addr)[0]
> > +
> > +        mem = self.phys_memory_read(addr, 16)
> > +        (version, addr, size) = struct.unpack("<IQI", mem)
> > +        if version != 0:
> > +            return
> > +
> > +        vmcoreinfo = self.phys_memory_read(addr, size)
> > +        if vmcoreinfo:
> > +            self.elf.add_vmcoreinfo_note(vmcoreinfo.tobytes())
> > +
> >      def invoke(self, args, from_tty):
> >          """Handles command invocation from gdb."""
> >  
> > @@ -518,6 +557,7 @@ shape and this command should mostly work."""
> >  
> >          self.elf = ELF(argv[1])
> >          self.guest_phys_blocks = get_guest_phys_blocks()
> > +        self.add_vmcoreinfo()
> >  
> >          with open(argv[0], "wb") as vmcore:
> >              self.dump_init(vmcore)
> > diff --git a/hw/acpi/vmcoreinfo.c b/hw/acpi/vmcoreinfo.c
> > index 0ea41de8d9..b6bcb47506 100644
> > --- a/hw/acpi/vmcoreinfo.c
> > +++ b/hw/acpi/vmcoreinfo.c
> > @@ -163,6 +163,8 @@ static void vmcoreinfo_handle_reset(void *opaque)
> >      memset(vis->vmcoreinfo_addr_le, 0,
> >      ARRAY_SIZE(vis->vmcoreinfo_addr_le));
> >  }
> >  
> > +static VMCoreInfoState *vmcoreinfo_gdb_helper;
> > +
> >  static void vmcoreinfo_realize(DeviceState *dev, Error **errp)
> >  {
> >      if (!bios_linker_loader_can_write_pointer()) {
> > @@ -181,6 +183,7 @@ static void vmcoreinfo_realize(DeviceState *dev, Error
> > **errp)
> >          return;
> >      }
> >  
> > +    vmcoreinfo_gdb_helper = VMCOREINFO(dev);
> >      qemu_register_reset(vmcoreinfo_handle_reset, dev);
> >  }
> >  
> > 
> 
> I guess we don't build QEMU with link-time optimization at the moment.
> 
> With link-time optimization, I think gcc might reasonably optimize away
> the assignment to "vmcoreinfo_gdb_helper", and "vmcoreinfo_gdb_helper"
> itself. This is why I suggested "volatile":
> 
> static VMCoreInfoState * volatile vmcoreinfo_gdb_helper;
> 
> Do you think volatile is only superfluous, or do you actively dislike it
> for some reason?

Yeah, I am not convinced volatile is the best way, but nor is static.

Let's export it?

On 07/11/17 12:04, Marc-André Lureau wrote:
> Hi
>
> ----- Original Message -----
>> On 07/06/17 12:16, Marc-André Lureau wrote:
>>> Add vmcoreinfo ELF note if vmcoreinfo device is ready.
>>>
>>> To help the python script, add a little global vmcoreinfo_gdb
>>> structure, that is populated with vmcoreinfo_gdb_update().
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

>>> @@ -181,6 +183,7 @@ static void vmcoreinfo_realize(DeviceState *dev, Error
>>> **errp)
>>>          return;
>>>      }
>>>
>>> +    vmcoreinfo_gdb_helper = VMCOREINFO(dev);
>>>      qemu_register_reset(vmcoreinfo_handle_reset, dev);
>>>  }
>>>
>>>
>>
>> I guess we don't build QEMU with link-time optimization at the
>> moment.
>>
>> With link-time optimization, I think gcc might reasonably optimize
>> away the assignment to "vmcoreinfo_gdb_helper", and
>> "vmcoreinfo_gdb_helper" itself. This is why I suggested "volatile":
>>
>> static VMCoreInfoState * volatile vmcoreinfo_gdb_helper;
>>
>> Do you think volatile is only superfluous, or do you actively dislike
>> it for some reason?
>
> Yeah, I am not convinced volatile is the best way, but nor is static.
>
> Let's export it?

Volatile guarantees that the assignment will take place according to the
behavior of the "abstract C machine" described by ISO C. From ISO C99,

  6.7.3 Type qualifiers

  6 An object that has volatile-qualified type may be modified in ways
    unknown to the implementation or have other unknown side effects.
    Therefore any expression referring to such an object shall be
    evaluated strictly according to the rules of the abstract machine,
    as described in 5.1.2.3. Furthermore, at every sequence point the
    value last stored in the object shall agree with that prescribed by
    the abstract machine, except as modified by the unknown factors
    mentioned previously. [116] What constitutes an access to an object
    that has volatile-qualified type is implementation-defined.

  Footnote 116: A volatile declaration may be used to describe an object
                corresponding to a memory-mapped input/output port or an
                object accessed by an asynchronously interrupting
                function. Actions on objects so declared shall not be
                "optimized out" by an implementation or reordered except
                as permitted by the rules for evaluating expressions.

So basically if you can find the symbol somehow, it will always have the
right value, due to the volatile qualifier, regardless of the
optimizations the compiler did.

Internal vs. external linkage ("static" vs. "extern") is a different
question; it might affect whether you find the symbol at all. IME,
symbols for objects with internal linkage are preserved, if: (a) you
don't strip the final binary, and (b) gcc doesn't optimize the variable
away.

With link-time / full program optimization, gcc is entitled to optimize
away variables with external linkage too, so "extern" in itself is not
bulletproof. Volatile is more important.

Given volatile, I'm sort of neutral on extern vs. static. However, if
you make the variable extern, that makes abuse from other source files
easier. (The variable should only be looked at with gdb.) This is also
why I originally suggested to limit the scope of even the static
variable to function scope -- this way abuse would completely be
prevented (nothing else could access the variable even from the same
translation unit), and gdb would still find the variable (IME).

Thanks
Laszlo

Hi

----- Original Message -----
> On 07/11/17 12:04, Marc-André Lureau wrote:
> > Hi
> >
> > ----- Original Message -----
> >> On 07/06/17 12:16, Marc-André Lureau wrote:
> >>> Add vmcoreinfo ELF note if vmcoreinfo device is ready.
> >>>
> >>> To help the python script, add a little global vmcoreinfo_gdb
> >>> structure, that is populated with vmcoreinfo_gdb_update().
> >>>
> >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> >>> @@ -181,6 +183,7 @@ static void vmcoreinfo_realize(DeviceState *dev,
> >>> Error
> >>> **errp)
> >>>          return;
> >>>      }
> >>>
> >>> +    vmcoreinfo_gdb_helper = VMCOREINFO(dev);
> >>>      qemu_register_reset(vmcoreinfo_handle_reset, dev);
> >>>  }
> >>>
> >>>
> >>
> >> I guess we don't build QEMU with link-time optimization at the
> >> moment.
> >>
> >> With link-time optimization, I think gcc might reasonably optimize
> >> away the assignment to "vmcoreinfo_gdb_helper", and
> >> "vmcoreinfo_gdb_helper" itself. This is why I suggested "volatile":
> >>
> >> static VMCoreInfoState * volatile vmcoreinfo_gdb_helper;
> >>
> >> Do you think volatile is only superfluous, or do you actively dislike
> >> it for some reason?
> >
> > Yeah, I am not convinced volatile is the best way, but nor is static.
> >
> > Let's export it?
> 
> Volatile guarantees that the assignment will take place according to the
> behavior of the "abstract C machine" described by ISO C. From ISO C99,
> 
>   6.7.3 Type qualifiers
> 
>   6 An object that has volatile-qualified type may be modified in ways
>     unknown to the implementation or have other unknown side effects.
>     Therefore any expression referring to such an object shall be
>     evaluated strictly according to the rules of the abstract machine,
>     as described in 5.1.2.3. Furthermore, at every sequence point the
>     value last stored in the object shall agree with that prescribed by
>     the abstract machine, except as modified by the unknown factors
>     mentioned previously. [116] What constitutes an access to an object
>     that has volatile-qualified type is implementation-defined.
> 
>   Footnote 116: A volatile declaration may be used to describe an object
>                 corresponding to a memory-mapped input/output port or an
>                 object accessed by an asynchronously interrupting
>                 function. Actions on objects so declared shall not be
>                 "optimized out" by an implementation or reordered except
>                 as permitted by the rules for evaluating expressions.
> 
> So basically if you can find the symbol somehow, it will always have the
> right value, due to the volatile qualifier, regardless of the
> optimizations the compiler did.
> 
> Internal vs. external linkage ("static" vs. "extern") is a different
> question; it might affect whether you find the symbol at all. IME,
> symbols for objects with internal linkage are preserved, if: (a) you
> don't strip the final binary, and (b) gcc doesn't optimize the variable
> away.
> 
> With link-time / full program optimization, gcc is entitled to optimize
> away variables with external linkage too, so "extern" in itself is not
> bulletproof. Volatile is more important.

Ok, you seem confident about this sort of things, I am not, I'll follow you :)
> 

> Given volatile, I'm sort of neutral on extern vs. static. However, if
> you make the variable extern, that makes abuse from other source files
> easier. (The variable should only be looked at with gdb.) This is also
> why I originally suggested to limit the scope of even the static
> variable to function scope -- this way abuse would completely be
> prevented (nothing else could access the variable even from the same
> translation unit), and gdb would still find the variable (IME).

How do you access a static inside a function? Gdb can look it up but complains that it's not in current context.

On 07/11/17 15:35, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
>> On 07/11/17 12:04, Marc-André Lureau wrote:
>>> Hi
>>>
>>> ----- Original Message -----
>>>> On 07/06/17 12:16, Marc-André Lureau wrote:
>>>>> Add vmcoreinfo ELF note if vmcoreinfo device is ready.
>>>>>
>>>>> To help the python script, add a little global vmcoreinfo_gdb
>>>>> structure, that is populated with vmcoreinfo_gdb_update().
>>>>>
>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>>>>> @@ -181,6 +183,7 @@ static void vmcoreinfo_realize(DeviceState *dev,
>>>>> Error
>>>>> **errp)
>>>>>          return;
>>>>>      }
>>>>>
>>>>> +    vmcoreinfo_gdb_helper = VMCOREINFO(dev);
>>>>>      qemu_register_reset(vmcoreinfo_handle_reset, dev);
>>>>>  }
>>>>>
>>>>>
>>>>
>>>> I guess we don't build QEMU with link-time optimization at the
>>>> moment.
>>>>
>>>> With link-time optimization, I think gcc might reasonably optimize
>>>> away the assignment to "vmcoreinfo_gdb_helper", and
>>>> "vmcoreinfo_gdb_helper" itself. This is why I suggested "volatile":
>>>>
>>>> static VMCoreInfoState * volatile vmcoreinfo_gdb_helper;
>>>>
>>>> Do you think volatile is only superfluous, or do you actively dislike
>>>> it for some reason?
>>>
>>> Yeah, I am not convinced volatile is the best way, but nor is static.
>>>
>>> Let's export it?
>>
>> Volatile guarantees that the assignment will take place according to the
>> behavior of the "abstract C machine" described by ISO C. From ISO C99,
>>
>>   6.7.3 Type qualifiers
>>
>>   6 An object that has volatile-qualified type may be modified in ways
>>     unknown to the implementation or have other unknown side effects.
>>     Therefore any expression referring to such an object shall be
>>     evaluated strictly according to the rules of the abstract machine,
>>     as described in 5.1.2.3. Furthermore, at every sequence point the
>>     value last stored in the object shall agree with that prescribed by
>>     the abstract machine, except as modified by the unknown factors
>>     mentioned previously. [116] What constitutes an access to an object
>>     that has volatile-qualified type is implementation-defined.
>>
>>   Footnote 116: A volatile declaration may be used to describe an object
>>                 corresponding to a memory-mapped input/output port or an
>>                 object accessed by an asynchronously interrupting
>>                 function. Actions on objects so declared shall not be
>>                 "optimized out" by an implementation or reordered except
>>                 as permitted by the rules for evaluating expressions.
>>
>> So basically if you can find the symbol somehow, it will always have the
>> right value, due to the volatile qualifier, regardless of the
>> optimizations the compiler did.
>>
>> Internal vs. external linkage ("static" vs. "extern") is a different
>> question; it might affect whether you find the symbol at all. IME,
>> symbols for objects with internal linkage are preserved, if: (a) you
>> don't strip the final binary, and (b) gcc doesn't optimize the variable
>> away.
>>
>> With link-time / full program optimization, gcc is entitled to optimize
>> away variables with external linkage too, so "extern" in itself is not
>> bulletproof. Volatile is more important.
> 
> Ok, you seem confident about this sort of things, I am not, I'll follow you :)
>>
> 
>> Given volatile, I'm sort of neutral on extern vs. static. However, if
>> you make the variable extern, that makes abuse from other source files
>> easier. (The variable should only be looked at with gdb.) This is also
>> why I originally suggested to limit the scope of even the static
>> variable to function scope -- this way abuse would completely be
>> prevented (nothing else could access the variable even from the same
>> translation unit), and gdb would still find the variable (IME).
> 
> How do you access a static inside a function? Gdb can look it up but complains that it's not in current context.
> 

According to <https://sourceware.org/gdb/onlinedocs/gdb/Variables.html>,
the magic incantation is

  function::variable

If this doesn't work (in case the realize function isn't on the stack at
all), then I agree the variable should be made file scope.

Thanks
Laszlo

Hi

----- Original Message -----
> On 07/11/17 15:35, Marc-André Lureau wrote:
> > Hi
> > 
> > ----- Original Message -----
> >> On 07/11/17 12:04, Marc-André Lureau wrote:
> >>> Hi
> >>>
> >>> ----- Original Message -----
> >>>> On 07/06/17 12:16, Marc-André Lureau wrote:
> >>>>> Add vmcoreinfo ELF note if vmcoreinfo device is ready.
> >>>>>
> >>>>> To help the python script, add a little global vmcoreinfo_gdb
> >>>>> structure, that is populated with vmcoreinfo_gdb_update().
> >>>>>
> >>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>
> >>>>> @@ -181,6 +183,7 @@ static void vmcoreinfo_realize(DeviceState *dev,
> >>>>> Error
> >>>>> **errp)
> >>>>>          return;
> >>>>>      }
> >>>>>
> >>>>> +    vmcoreinfo_gdb_helper = VMCOREINFO(dev);
> >>>>>      qemu_register_reset(vmcoreinfo_handle_reset, dev);
> >>>>>  }
> >>>>>
> >>>>>
> >>>>
> >>>> I guess we don't build QEMU with link-time optimization at the
> >>>> moment.
> >>>>
> >>>> With link-time optimization, I think gcc might reasonably optimize
> >>>> away the assignment to "vmcoreinfo_gdb_helper", and
> >>>> "vmcoreinfo_gdb_helper" itself. This is why I suggested "volatile":
> >>>>
> >>>> static VMCoreInfoState * volatile vmcoreinfo_gdb_helper;
> >>>>
> >>>> Do you think volatile is only superfluous, or do you actively dislike
> >>>> it for some reason?
> >>>
> >>> Yeah, I am not convinced volatile is the best way, but nor is static.
> >>>
> >>> Let's export it?
> >>
> >> Volatile guarantees that the assignment will take place according to the
> >> behavior of the "abstract C machine" described by ISO C. From ISO C99,
> >>
> >>   6.7.3 Type qualifiers
> >>
> >>   6 An object that has volatile-qualified type may be modified in ways
> >>     unknown to the implementation or have other unknown side effects.
> >>     Therefore any expression referring to such an object shall be
> >>     evaluated strictly according to the rules of the abstract machine,
> >>     as described in 5.1.2.3. Furthermore, at every sequence point the
> >>     value last stored in the object shall agree with that prescribed by
> >>     the abstract machine, except as modified by the unknown factors
> >>     mentioned previously. [116] What constitutes an access to an object
> >>     that has volatile-qualified type is implementation-defined.
> >>
> >>   Footnote 116: A volatile declaration may be used to describe an object
> >>                 corresponding to a memory-mapped input/output port or an
> >>                 object accessed by an asynchronously interrupting
> >>                 function. Actions on objects so declared shall not be
> >>                 "optimized out" by an implementation or reordered except
> >>                 as permitted by the rules for evaluating expressions.
> >>
> >> So basically if you can find the symbol somehow, it will always have the
> >> right value, due to the volatile qualifier, regardless of the
> >> optimizations the compiler did.
> >>
> >> Internal vs. external linkage ("static" vs. "extern") is a different
> >> question; it might affect whether you find the symbol at all. IME,
> >> symbols for objects with internal linkage are preserved, if: (a) you
> >> don't strip the final binary, and (b) gcc doesn't optimize the variable
> >> away.
> >>
> >> With link-time / full program optimization, gcc is entitled to optimize
> >> away variables with external linkage too, so "extern" in itself is not
> >> bulletproof. Volatile is more important.
> > 
> > Ok, you seem confident about this sort of things, I am not, I'll follow you
> > :)
> >>
> > 
> >> Given volatile, I'm sort of neutral on extern vs. static. However, if
> >> you make the variable extern, that makes abuse from other source files
> >> easier. (The variable should only be looked at with gdb.) This is also
> >> why I originally suggested to limit the scope of even the static
> >> variable to function scope -- this way abuse would completely be
> >> prevented (nothing else could access the variable even from the same
> >> translation unit), and gdb would still find the variable (IME).
> > 
> > How do you access a static inside a function? Gdb can look it up but
> > complains that it's not in current context.
> > 
> 
> According to <https://sourceware.org/gdb/onlinedocs/gdb/Variables.html>,
> the magic incantation is
> 
>   function::variable
> 

Thanks that works! I'll update the patch

> If this doesn't work (in case the realize function isn't on the stack at
> all), then I agree the variable should be made file scope.
> 

Why would it matter if realize is on the stack? It's likely not :)

On 07/11/17 15:58, Marc-André Lureau wrote:

>>> How do you access a static inside a function? Gdb can look it up but
>>> complains that it's not in current context.
>>>
>>
>> According to <https://sourceware.org/gdb/onlinedocs/gdb/Variables.html>,
>> the magic incantation is
>>
>>   function::variable
>>
> 
> Thanks that works! I'll update the patch

Great!

> 
>> If this doesn't work (in case the realize function isn't on the stack at
>> all), then I agree the variable should be made file scope.
>>
> 
> Why would it matter if realize is on the stack? It's likely not :)

I agree the realize function will likely not be on the stack, and I also
agree that it should not matter. However, the gdb docs seemed a bit
unclear to me regarding this, so I wasn't 100% sure without trying.

Thanks
Laszlo