diff mbox series

[v1,6/6] tests/acceptance: pick a random gdb port for reverse debugging

Message ID 20201021163136.27324-7-alex.bennee@linaro.org
State New
Headers show
Series testing/next (gitdm, acceptance, docker, gitlab) | expand

Commit Message

Alex Bennée Oct. 21, 2020, 4:31 p.m. UTC 
Currently the test randomly fails if you are using a shared machine
due to contention on the well known port 1234. We can ameliorate this
a bit by picking a random non-ephemeral port although it doesn't
totally avoid the problem. While we could use a totally unique socket
address for debugging it's impossible to probe for gdb support of the
feature which makes this a sub-optimal but less fiddly option.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/acceptance/reverse_debugging.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Thomas Huth Oct. 22, 2020, 5:20 a.m. UTC | #1 
On 21/10/2020 18.31, Alex Bennée wrote:
> Currently the test randomly fails if you are using a shared machine
> due to contention on the well known port 1234. We can ameliorate this
> a bit by picking a random non-ephemeral port although it doesn't
> totally avoid the problem. While we could use a totally unique socket
> address for debugging it's impossible to probe for gdb support of the
> feature which makes this a sub-optimal but less fiddly option.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/acceptance/reverse_debugging.py | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Certainly better than before!

Reviewed-by: Thomas Huth <thuth@redhat.com>
Pavel Dovgalyuk Oct. 22, 2020, 6:18 a.m. UTC | #2 
On 21.10.2020 19:31, Alex Bennée wrote:
> Currently the test randomly fails if you are using a shared machine

> due to contention on the well known port 1234. We can ameliorate this

> a bit by picking a random non-ephemeral port although it doesn't

> totally avoid the problem. While we could use a totally unique socket

> address for debugging it's impossible to probe for gdb support of the

> feature which makes this a sub-optimal but less fiddly option.

> 

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>


Reviewed-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>


> ---

>   tests/acceptance/reverse_debugging.py | 6 ++++--

>   1 file changed, 4 insertions(+), 2 deletions(-)

> 

> diff --git a/tests/acceptance/reverse_debugging.py b/tests/acceptance/reverse_debugging.py

> index b72fdf6cdc..f2e8245471 100644

> --- a/tests/acceptance/reverse_debugging.py

> +++ b/tests/acceptance/reverse_debugging.py

> @@ -16,6 +16,7 @@ from avocado.utils import gdb

>   from avocado.utils import process

>   from avocado.utils.path import find_command

>   from boot_linux_console import LinuxKernelTest

> +from random import randrange

>   

>   class ReverseDebugging(LinuxKernelTest):

>       """

> @@ -43,7 +44,8 @@ class ReverseDebugging(LinuxKernelTest):

>           else:

>               logger.info('replaying the execution...')

>               mode = 'replay'

> -            vm.add_args('-s', '-S')

> +            self.port = randrange(2048, 49152)

> +            vm.add_args('-gdb', 'tcp::%d' % (self.port), '-S')

>           vm.add_args('-icount', 'shift=%s,rr=%s,rrfile=%s,rrsnapshot=init' %

>                       (shift, mode, replay_path),

>                       '-net', 'none')

> @@ -122,7 +124,7 @@ class ReverseDebugging(LinuxKernelTest):

>           # replay and run debug commands

>           vm = self.run_vm(False, shift, args, replay_path, image_path)

>           logger.info('connecting to gdbstub')

> -        g = gdb.GDBRemote('127.0.0.1', 1234, False, False)

> +        g = gdb.GDBRemote('127.0.0.1', self.port, False, False)

>           g.connect()

>           r = g.cmd(b'qSupported')

>           if b'qXfer:features:read+' in r:

>
Philippe Mathieu-Daudé Oct. 22, 2020, 11:07 a.m. UTC | #3 
On 10/22/20 7:20 AM, Thomas Huth wrote:
> On 21/10/2020 18.31, Alex Bennée wrote:

>> Currently the test randomly fails if you are using a shared machine

>> due to contention on the well known port 1234. We can ameliorate this

>> a bit by picking a random non-ephemeral port although it doesn't

>> totally avoid the problem. While we could use a totally unique socket

>> address for debugging it's impossible to probe for gdb support of the

>> feature which makes this a sub-optimal but less fiddly option.

>>

>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

>> ---

>>   tests/acceptance/reverse_debugging.py | 6 ++++--

>>   1 file changed, 4 insertions(+), 2 deletions(-)

> 

> Certainly better than before!


I'd prefer another chardev that tcp, but as you said this is
already an improvement, so:

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


> 

> Reviewed-by: Thomas Huth <thuth@redhat.com>

>
Alex Bennée Oct. 22, 2020, 1:07 p.m. UTC | #4 
Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 10/22/20 7:20 AM, Thomas Huth wrote:
>> On 21/10/2020 18.31, Alex Bennée wrote:
>>> Currently the test randomly fails if you are using a shared machine
>>> due to contention on the well known port 1234. We can ameliorate this
>>> a bit by picking a random non-ephemeral port although it doesn't
>>> totally avoid the problem. While we could use a totally unique socket
>>> address for debugging it's impossible to probe for gdb support of the
>>> feature which makes this a sub-optimal but less fiddly option.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>>   tests/acceptance/reverse_debugging.py | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>> 
>> Certainly better than before!
>
> I'd prefer another chardev that tcp, but as you said this is
> already an improvement, so:

We've supported sockets gdb and softmmu emulation for some time:

  -gdb unix:path=gdb.sock,server

the trouble is detecting if the host installed gdb is going to connect
before we try and fail after launching the VM. I think we might get away
with a version probe:

  > luispm: I want to know ahead of time for my scripts if gdb can do a
      "target remote gdb.sock"
  <luispm> ajb-linaro, I don't think so. My guess is that GDB will
      always attempt to stablish a connection if the socket is valid.
  <luispm> ajb-linaro, Can you check the validity of the file before
      invoking GDB?
  <luispm> ajb-linaro, No concept of "is this particular remote target
      available?".
  > luispm: it's not that - I know the socket will exist but the older
      gdb just bombs out trying to read it.
  <luispm> ajb-linaro, Not good.
  <luispm> ajb-linaro, So is this a matter of an older GDB that doesn't
      support using socket files and a newer one that does?
  > luispm: I thought I might probe "help target remote" text but it's
      unchanged between versions
  > luispm: yes
  <luispm> ajb-linaro, I think the code is probably hidden within the
      "target remote" implementation.
  > luispm: and most distro gdb's don't at the moment
  > luispm: if I could work out the version it was added that might help
  <luispm> ajb-linaro, I see some bits of it were reverted at some
      point.
  <luispm> ajb-linaro, Let me check.
  <luispm> ajb-linaro, It looks like GDB 8.3 was the first stable to get
      it.
  > luispm: thanks - I'll see if I can script that up
  <luispm> ajb-linaro, Looks like they initially went with an explicit
      prefix of "unix:" before the socket. But then dropped that in
      favor of autodetecting the socket file.
  <luispm> ajb-linaro, The only thing I get for a GDB that doesn't
      support socket connections if
      "/run/user/1000/at-spi2-QTZBS0/socket: No such device or address."
  <luispm> *is*
  <luispm> This is 8.1 in Ubuntu 18.04.
  <luispm> master GDB says "Remote communication error.  Target
      disconnected.: Connection reset by peer."

>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
>> 
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>
Cleber Rosa Oct. 22, 2020, 3:46 p.m. UTC | #5 
On Wed, Oct 21, 2020 at 05:31:36PM +0100, Alex Bennée wrote:
> Currently the test randomly fails if you are using a shared machine

> due to contention on the well known port 1234. We can ameliorate this

> a bit by picking a random non-ephemeral port although it doesn't

> totally avoid the problem. While we could use a totally unique socket

> address for debugging it's impossible to probe for gdb support of the

> feature which makes this a sub-optimal but less fiddly option.

>


Hi Alex,

This is already a clear improvement, so consider my points as suggestions.

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> ---

>  tests/acceptance/reverse_debugging.py | 6 ++++--

>  1 file changed, 4 insertions(+), 2 deletions(-)

> 

> diff --git a/tests/acceptance/reverse_debugging.py b/tests/acceptance/reverse_debugging.py

> index b72fdf6cdc..f2e8245471 100644

> --- a/tests/acceptance/reverse_debugging.py

> +++ b/tests/acceptance/reverse_debugging.py

> @@ -16,6 +16,7 @@ from avocado.utils import gdb

>  from avocado.utils import process

>  from avocado.utils.path import find_command

>  from boot_linux_console import LinuxKernelTest

> +from random import randrange


Avocado ships with a "avocado.utils.network.ports.find_free_port" utility:

   https://avocado-framework.readthedocs.io/en/81.0/api/utils/avocado.utils.network.html#avocado.utils.network.ports.find_free_port

Which *minimizes* the possibility of a clash by checking if the port
is available.  I think it's worth to consider using it.

>  

>  class ReverseDebugging(LinuxKernelTest):

>      """

> @@ -43,7 +44,8 @@ class ReverseDebugging(LinuxKernelTest):

>          else:

>              logger.info('replaying the execution...')

>              mode = 'replay'

> -            vm.add_args('-s', '-S')

> +            self.port = randrange(2048, 49152)

> +            vm.add_args('-gdb', 'tcp::%d' % (self.port), '-S')


It's a good idea to try to avoid setting instance attributes outside
of __init__().  In this specific case, I'd just add a "port" parameter
to run_vm().

>          vm.add_args('-icount', 'shift=%s,rr=%s,rrfile=%s,rrsnapshot=init' %

>                      (shift, mode, replay_path),

>                      '-net', 'none')

> @@ -122,7 +124,7 @@ class ReverseDebugging(LinuxKernelTest):

>          # replay and run debug commands

>          vm = self.run_vm(False, shift, args, replay_path, image_path)

>          logger.info('connecting to gdbstub')

> -        g = gdb.GDBRemote('127.0.0.1', 1234, False, False)

> +        g = gdb.GDBRemote('127.0.0.1', self.port, False, False)

>          g.connect()

>          r = g.cmd(b'qSupported')

>          if b'qXfer:features:read+' in r:

> -- 

> 2.20.1

> 



So, the overall diff of my suggestions look like:

---
diff --git a/tests/acceptance/reverse_debugging.py b/tests/acceptance/reverse_debugging.py
index f2e8245471..3d91dfaa8f 100644
--- a/tests/acceptance/reverse_debugging.py
+++ b/tests/acceptance/reverse_debugging.py
@@ -14,9 +14,10 @@ from avocado import skipIf
 from avocado_qemu import BUILD_DIR
 from avocado.utils import gdb
 from avocado.utils import process
+from avocado.utils.network.ports import find_free_port
 from avocado.utils.path import find_command
 from boot_linux_console import LinuxKernelTest
-from random import randrange
+
 
 class ReverseDebugging(LinuxKernelTest):
     """
@@ -34,7 +35,7 @@ class ReverseDebugging(LinuxKernelTest):
     STEPS = 10
     endian_is_le = True
 
-    def run_vm(self, record, shift, args, replay_path, image_path):
+    def run_vm(self, record, shift, args, replay_path, image_path, port):
         logger = logging.getLogger('replay')
         vm = self.get_vm()
         vm.set_console()
@@ -44,8 +45,7 @@ class ReverseDebugging(LinuxKernelTest):
         else:
             logger.info('replaying the execution...')
             mode = 'replay'
-            self.port = randrange(2048, 49152)
-            vm.add_args('-gdb', 'tcp::%d' % (self.port), '-S')
+            vm.add_args('-gdb', 'tcp::%d' % port, '-S')
         vm.add_args('-icount', 'shift=%s,rr=%s,rrfile=%s,rrsnapshot=init' %
                     (shift, mode, replay_path),
                     '-net', 'none')
@@ -111,9 +111,10 @@ class ReverseDebugging(LinuxKernelTest):
         process.run(cmd)
 
         replay_path = os.path.join(self.workdir, 'replay.bin')
+        port = find_free_port()
 
         # record the log
-        vm = self.run_vm(True, shift, args, replay_path, image_path)
+        vm = self.run_vm(True, shift, args, replay_path, image_path, port)
         while self.vm_get_icount(vm) <= self.STEPS:
             pass
         last_icount = self.vm_get_icount(vm)
@@ -122,9 +123,9 @@ class ReverseDebugging(LinuxKernelTest):
         logger.info("recorded log with %s+ steps" % last_icount)
 
         # replay and run debug commands
-        vm = self.run_vm(False, shift, args, replay_path, image_path)
+        vm = self.run_vm(False, shift, args, replay_path, image_path, port)
         logger.info('connecting to gdbstub')
-        g = gdb.GDBRemote('127.0.0.1', self.port, False, False)
+        g = gdb.GDBRemote('127.0.0.1', port, False, False)
         g.connect()
         r = g.cmd(b'qSupported')
         if b'qXfer:features:read+' in r:
---

Regards,
- Cleber.
diff mbox series

Patch

diff --git a/tests/acceptance/reverse_debugging.py b/tests/acceptance/reverse_debugging.py
index b72fdf6cdc..f2e8245471 100644
--- a/tests/acceptance/reverse_debugging.py
+++ b/tests/acceptance/reverse_debugging.py
@@ -16,6 +16,7 @@  from avocado.utils import gdb
 from avocado.utils import process
 from avocado.utils.path import find_command
 from boot_linux_console import LinuxKernelTest
+from random import randrange
 
 class ReverseDebugging(LinuxKernelTest):
     """
@@ -43,7 +44,8 @@  class ReverseDebugging(LinuxKernelTest):
         else:
             logger.info('replaying the execution...')
             mode = 'replay'
-            vm.add_args('-s', '-S')
+            self.port = randrange(2048, 49152)
+            vm.add_args('-gdb', 'tcp::%d' % (self.port), '-S')
         vm.add_args('-icount', 'shift=%s,rr=%s,rrfile=%s,rrsnapshot=init' %
                     (shift, mode, replay_path),
                     '-net', 'none')
@@ -122,7 +124,7 @@  class ReverseDebugging(LinuxKernelTest):
         # replay and run debug commands
         vm = self.run_vm(False, shift, args, replay_path, image_path)
         logger.info('connecting to gdbstub')
-        g = gdb.GDBRemote('127.0.0.1', 1234, False, False)
+        g = gdb.GDBRemote('127.0.0.1', self.port, False, False)
         g.connect()
         r = g.cmd(b'qSupported')
         if b'qXfer:features:read+' in r: