Re: [Lldb-commits] [PATCH] D12677: Bug 24733: TestRegisters.py for Clang inferiors

[Oleksiy Vyalov via lldb-commits]

ovyalov added a comment.

In http://reviews.llvm.org/D12677#242290, @abhishek.aggarwal wrote:

> In http://reviews.llvm.org/D12677#242006, @ovyalov wrote:
>
> > I reverted the CL because it was causing 
> > TestRegisters.test_fp_special_purpose_register_read to fail on OSX:
> >
> > - stop reason = EXC_BREAKPOINT
> > - "register read ftag" yields 0x80 instead of expected 0x8000
>
>
>


Hi Abhishek,

please see my comments inline:

> Hi Oleksiy

> 

> Thanks for pointing out.  Two things:

> 

> 1. Is it possible for you to share the failure log for this test on MacOSX? 
> The expected value of ftag is 0x0080 and not 0x8000. Which compiler was used 
> to compile inferior?


F818989: TestRegisters.txt 

I'm using clang 3.6

> 2. Do you think that this patch needs reversion? In my opinion, this patch 
> provides a much better (genric) way to test X87 FPU on Linux OS (atleast). I 
> don't know whether the status for MacOSX would have even changed after you 
> reverted this patch. I believe, it should still fail for MacOSX. I can take a 
> look for MacOSX once you provide me failure logs. I would recommend to keep 
> this patch and for the time being skip it for MacOSX. Once I find a fix for 
> it on MacOSX, we can enable it for the same as well. What do you think ?


It makes sense to me to split fix in a few iterations:

1. Submit the patch as-is just with XFAIL for Darwin (please check whether 
llvm.org/pr24733 is still relevant or it's failing on OSX due another error).
2. Make a fix for Darwin if possible.


http://reviews.llvm.org/D12677



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D12677: Bug 24733: TestRegisters.py for Clang inferiors

[Abhishek via lldb-commits]

abhishek.aggarwal added a comment.

In http://reviews.llvm.org/D12677#242006, @ovyalov wrote:

> I reverted the CL because it was causing 
> TestRegisters.test_fp_special_purpose_register_read to fail on OSX:
>
> - stop reason = EXC_BREAKPOINT
> - "register read ftag" yields 0x80 instead of expected 0x8000


Hi Oleksiy

Thanks for pointing out.  Two things:

1. Is it possible for you to share the failure log for this test on MacOSX? The 
expected value of ftag is 0x0080 and not 0x8000. Which compiler was used to 
compile inferior?
2. Do you think that this patch needs reversion? In my opinion, this patch 
provides a much better (genric) way to test X87 FPU on Linux OS (atleast). I 
don't know whether the status for MacOSX would have even changed after you 
reverted this patch. I believe, it should still fail for MacOSX. I can take a 
look for MacOSX once you provide me failure logs. I would recommend to keep 
this patch and for the time being skip it for MacOSX. Once I find a fix for it 
on MacOSX, we can enable it for the same as well. What do you think ?


http://reviews.llvm.org/D12677



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D12677: Bug 24733: TestRegisters.py for Clang inferiors

[Oleksiy Vyalov via lldb-commits]

ovyalov added a subscriber: ovyalov.
ovyalov added a comment.

I reverted the CL because it was causing 
TestRegisters.test_fp_special_purpose_register_read to fail on OSX:

- stop reason = EXC_BREAKPOINT
- "register read ftag" yields 0x80 instead of expected 0x8000




http://reviews.llvm.org/D12677



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D12677: Bug 24733: TestRegisters.py for Clang inferiors

[Abhishek via lldb-commits]

abhishek.aggarwal closed this revision.
abhishek.aggarwal added a comment.

Couldn't land this patch by arc due to merge conflicts. Hence merging it 
manually and closing this revision.


http://reviews.llvm.org/D12677



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D12677: Bug 24733: TestRegisters.py for Clang inferiors

[Pavel Labath via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

looks good. please also close the relevant bug after submission.


http://reviews.llvm.org/D12677



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D12677: Bug 24733: TestRegisters.py for Clang inferiors

[Abhishek via lldb-commits]

abhishek.aggarwal updated this revision to Diff 34161.
abhishek.aggarwal added a comment.

Clang/GCC generate different assembly for same inferior.

Changed a.cpp to remove dependency of TestRegisters.py on assembly
instructions generated by Clang/GCC.

Changed TestRegisters.py to write a generic test vector for testing
Bug 24457.


http://reviews.llvm.org/D12677

Files:
  test/functionalities/register/TestRegisters.py
  test/functionalities/register/a.cpp

Index: test/functionalities/register/a.cpp
===
--- test/functionalities/register/a.cpp
+++ test/functionalities/register/a.cpp
@@ -13,6 +13,7 @@
 {
 float a=2, b=4,c=8, d=16, e=32, f=64, k=128, l=256, add=0;
 __asm__ (
+"int3 ;"
 "flds %1 ;"
 "flds %2 ;"
 "flds %3 ;"
Index: test/functionalities/register/TestRegisters.py
===
--- test/functionalities/register/TestRegisters.py
+++ test/functionalities/register/TestRegisters.py
@@ -36,7 +36,6 @@
 self.buildDefault()
 self.fp_register_write()
 
-@expectedFailureClang("llvm.org/pr24733")
 def test_fp_special_purpose_register_read(self):
 """Test commands that read fpu special purpose registers."""
 if not self.getArchitecture() in ['amd64', 'i386', 'x86_64']:
@@ -168,15 +167,14 @@
 target = self.dbg.CreateTarget(exe)
 self.assertTrue(target, VALID_TARGET)
 
-# Find the line number to break inside a.cpp.
-self.line = line_number('a.cpp', '// Set break point at this line.')
-
-# Set breakpoint
-lldbutil.run_break_set_by_file_and_line (self, "a.cpp", self.line, 
num_expected_locations=1, loc_exact=True)
-
 # Launch the process, and do not stop at the entry point.
 self.runCmd ("run", RUN_SUCCEEDED)
 
+# Check stop reason; Should be SIGTRAP
+stop_reason = 'stop reason = signal SIGTRAP'
+self.expect("thread list", STOPPED_DUE_TO_SIGNAL,
+  substrs = ['stopped', stop_reason])
+
 process = target.GetProcess()
 self.assertTrue(process.GetState() == lldb.eStateStopped,
 PROCESS_STOPPED)


Index: test/functionalities/register/a.cpp
===
--- test/functionalities/register/a.cpp
+++ test/functionalities/register/a.cpp
@@ -13,6 +13,7 @@
 {
 float a=2, b=4,c=8, d=16, e=32, f=64, k=128, l=256, add=0;
 __asm__ (
+"int3 ;"
 "flds %1 ;"
 "flds %2 ;"
 "flds %3 ;"
Index: test/functionalities/register/TestRegisters.py
===
--- test/functionalities/register/TestRegisters.py
+++ test/functionalities/register/TestRegisters.py
@@ -36,7 +36,6 @@
 self.buildDefault()
 self.fp_register_write()
 
-@expectedFailureClang("llvm.org/pr24733")
 def test_fp_special_purpose_register_read(self):
 """Test commands that read fpu special purpose registers."""
 if not self.getArchitecture() in ['amd64', 'i386', 'x86_64']:
@@ -168,15 +167,14 @@
 target = self.dbg.CreateTarget(exe)
 self.assertTrue(target, VALID_TARGET)
 
-# Find the line number to break inside a.cpp.
-self.line = line_number('a.cpp', '// Set break point at this line.')
-
-# Set breakpoint
-lldbutil.run_break_set_by_file_and_line (self, "a.cpp", self.line, num_expected_locations=1, loc_exact=True)
-
 # Launch the process, and do not stop at the entry point.
 self.runCmd ("run", RUN_SUCCEEDED)
 
+# Check stop reason; Should be SIGTRAP
+stop_reason = 'stop reason = signal SIGTRAP'
+self.expect("thread list", STOPPED_DUE_TO_SIGNAL,
+  substrs = ['stopped', stop_reason])
+
 process = target.GetProcess()
 self.assertTrue(process.GetState() == lldb.eStateStopped,
 PROCESS_STOPPED)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D12677: Bug 24733: TestRegisters.py for Clang inferiors

[Abhishek via lldb-commits]

abhishek.aggarwal added inline comments.


Comment at: test/functionalities/register/TestRegisters.py:195
@@ +194,3 @@
+for x in range(0,16):
+self.runCmd ("si", RUN_SUCCEEDED)
+

labath wrote:
> First I would like to applaud for writing a test case for such a delicate 
> issue. I know it's not easy given the current test infrastructure.
> 
> However, this change seems very fragile and likely to break due to random 
> changes in clang implementation and/or command line flags. Even the gcc path 
> can break if the gcc happens to produce slightly different output. I would 
> like to avoid relying on hardcoded instruction counts.
> 
> How about we try something like this:
> - in the inline assembly, we prepend the code you want to test with "int3"
> - run the inferior normally. it should hit the debugger trap and stop (you 
> can verify that the stop reason is indeed sigtrap)
> - the next instruction should point precisely at the code you want to test, 
> without relying on any debug info or instruction counts
> - proceed with the test normally
> 
> what do you think?
Thanks for suggesting a nice way to fix it. I agree with you. I will make the 
changes.


http://reviews.llvm.org/D12677



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D12677: Bug 24733: TestRegisters.py for Clang inferiors

[Pavel Labath via lldb-commits]

labath added a subscriber: lldb-commits.


Comment at: test/functionalities/register/TestRegisters.py:195
@@ +194,3 @@
+for x in range(0,16):
+self.runCmd ("si", RUN_SUCCEEDED)
+

First I would like to applaud for writing a test case for such a delicate 
issue. I know it's not easy given the current test infrastructure.

However, this change seems very fragile and likely to break due to random 
changes in clang implementation and/or command line flags. Even the gcc path 
can break if the gcc happens to produce slightly different output. I would like 
to avoid relying on hardcoded instruction counts.

How about we try something like this:
- in the inline assembly, we prepend the code you want to test with "int3"
- run the inferior normally. it should hit the debugger trap and stop (you can 
verify that the stop reason is indeed sigtrap)
- the next instruction should point precisely at the code you want to test, 
without relying on any debug info or instruction counts
- proceed with the test normally

what do you think?


http://reviews.llvm.org/D12677



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits