On 06/17/2013 03:56 PM, Denys Vlasenko wrote:
On 06/13/2013 04:55 PM, Martin Milata wrote:
Some remarks regarding the code below:

+    if self.operands.startswith("0x") or self.operands.startswith("-0x") or 
self.operands.startswith("("):
+        mem_op_pos = 0
+    else:
+        mem_op_pos = self.operands.find(",0x")
+        if mem_op_pos < 0:
+            mem_op_pos = self.operands.find(",-0x")
+        if mem_op_pos < 0:
+            mem_op_pos = self.operands.find(":0x")
+        if mem_op_pos < 0:
+            mem_op_pos = self.operands.find(":-0x")
+        if mem_op_pos < 0:
+            mem_op_pos = self.operands.find(",(")
+        if mem_op_pos < 0:
+            mem_op_pos = self.operands.find(":(")
+        if mem_op_pos < 0:
+            return False # no memory operands
+        mem_op_pos += 1
Can you use a regular expression for this, possibly with splitting the
operands into list first? http://docs.python.org/2.7/library/re.html

I think "import re", compiling of the regexp and matching
will be slower than searching for a string.

- even if the part about the speed would be true (which I doubt) we're not chasing for milliseconds...

It is also less understandable:

regex = re.compile('^\s+(0x[0-9a-f]+)\s+(\<\+\d+\>):\s+(.*)$')

I can't easily tell what this regexp is doing.


- you can use named groups to make it more clear:

('^\s+(?P<memop>0x[0-9a-f]+)\s+(?P<somethingelse>\<\+\d+\>):\s+(?P<rest>.*)$')

- with the groups it's quite clear what are you looking for..

The only benefit is that using regex may take a few lines less
than existing code.

- no, the benefit is that the regexp is more readable for 3 out of 4 people who saw that code so far..


Jirka


+        # HACK_ALERT: kernels before 3.?.? do not record siginfo in coredumps,
+        # so $_siginfo isn't present.
By the way, do you have any reference for this? The prstatus structure
which contains a signal number seems to be present in coredumps at least
on 2.6.32. Or am I misunderstanding the problem?

The point is, gdb needs _complete_ siginfo to be able to show $_siginfo.
A few bits of signal info in prstatus is a complete siginfo.
(Of course, $_siginfo.si_signo doesn't need complete siginfo,
but gdb doesn't know that...)

+    for line in raw_instructions.split("\n"):
+        # line can be:
+        # "Dump of assembler code from 0xAAAA to 0xBBBB:"
+        # "[=>] 0x00000000004004dc[ <+0>]:  push   %rbp"
+        #   (" <+0>" part is present when we run on a live process,
+        #   on coredump it is absent)
+        # "End of assembler dump."
+        # "" (empty line)
What about using regular expressions here?

re.compile('^\s+(0x[0-9a-f]+)\s+(\<\+\d+\>):\s+(.*)$')

The rest of your comments make sense, I'll update the branch.


Reply via email to