r131701 and test/CodeGenObjC/debug-property-synth.m

Clang Frontend
1

So I’m trying to do some stuff that’s affecting .loc directives and came across a couple of test cases in Clang that, as was the fashion at the time, test LLVM.

One of them is CodeGenObjC/debug-property-synth.m - which tests a particular .loc directive. The corresponding change it’s meant to test is r131701, which seems to actually contain two independent changes. http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGObjC.cpp?r1=131373&r2=131701&pathrev=131701&diff_format=h

  1. is the change at line 142 (and most of the changes in this patch - to pass the parameter through) to pass StartLoc instead of OMD->getLocStart()

  2. is the change at 492 to use PID instead of PD

The problems I have with upgrading this test is that (1) is untested by this test file (I would /really/ like to revert this functionality if it’s not tested, because I hate having unjustified complexity in the codebase - but perhaps you know of a way to test this piece of the patch?)

And (2) has changed a lot since the code was written (139466 addressed the fixme on the line above 492 - and removed anything that looks remotely like it). But I assume/hope that (2) is actually what the test case originally (and still does? maybe?) test.

Should I just remove the test case? Is there a logical way to update it to test what it was originally intended to test? (is that even still a valid thing to test after r139466 - I assume so?)

Thanks.

  • Dave
2

Gave up and deleted this test in r201183.

3

So I'm trying to do some stuff that's affecting .loc directives and came across a couple of test cases in Clang that, as was the fashion at the time, test LLVM.

One of them is CodeGenObjC/debug-property-synth.m - which tests a particular .loc directive. The corresponding change it's meant to test is r131701, which seems to actually contain two independent changes. http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGObjC.cpp?r1=131373&r2=131701&pathrev=131701&diff_format=h

Wonderfully documented testcase :-p

From looking at the radar, this test is supposed to test the location of synthesized getters and setters. We should not remove the testcase, but we should definitely make it more specific.

It should be testing that the synthesized setter/getter function for
     1 // RUN: %clang_cc1 -triple %itanium_abi_triple -masm-verbose -S -g %s -o - | FileCheck %s
     2 // Radar 9468526
     3 @interface I {
     4 int _p1;
     5 }
     6 @property int p1;
     7 @end
     8
     9 @implementation I
    10 @synthesize p1 = _p1;
    11 @end
    12
    13 int main() {
    14 I *myi;
    15 myi.p1 = 2;
    16 return 0;
    17 }
    18
    19 // FIXME: Make this test ir files.
    20 // CHECK: .loc 2 6 0

p1 are associated with line number 6.

1) is the change at line 142 (and most of the changes in this patch - to pass the parameter through) to pass StartLoc instead of OMD->getLocStart()

2) is the change at 492 to use PID instead of PD

What are the respective values for StartLoc versus OMD->getLocStart() for this example?

The problems I have with upgrading this test is that (1) is untested by this test file (I would /really/ like to revert this functionality if it's not tested, because I hate having unjustified complexity in the codebase - but perhaps you know of a way to test this piece of the patch?)

And (2) has changed a lot since the code was written (139466 addressed the fixme on the line above 492 - and removed anything that looks remotely like it). But I assume/hope that (2) is actually what the test case originally (and still does? maybe?) test.

Should I just remove the test case? Is there a logical way to update it to test what it was originally intended to test? (is that even still a valid thing to test after r139466 - I assume so?)

-- adrian

4

That's the problem - the code has since change substantially. Mostly in
r139468:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGObjC.cpp?r1=139466&r2=139468&diff_format=h

When I attempted to possibly reproduce the failure (by finding the one
place that r139468 kept a getLocStart call in GenerateObjCSetter, I think I
was only able to get debuginfo-properties.m to fail)

So my best guess is that this test became useless at some point (possibly
at 139468) due to refactoring and that the issue is currently covered by
debuginfo-properties.m.

I'm open to other ideas, though...

- David

5

AFAICT debuginfo-properties.m checks for the DW_AT_decl_line in the debug info, whereas this tests for a line table entry. I forgot to mention that the radar was about being able to set a breakpoint on the property.

-- adrian

6

OK - though I wasn't able to get the test case to fail. I suspect because
something else ended up with the same line information, perhaps? Or it
ended up with the correct line info through some other means? (I didn't
diff the full output to see if it changed)

The commit the test case was addressing didn't seem like it was doing
anything line-table specific... I assume it was just testing the line table
because it was a convenient way to test the line associated with this
construct (I assume this was originally a grep test long before
llvm-dwarfdump for example).

7

I’m rewriting the debug-property-synth.m test to be IR-based (and documented!) now — based on the description in the radar.
Passing the StartLoc all the way through affects both the line table location and the DW_AT_decl_line of the function (which is read from the CGDebugInfo::CurLoc).

void CodeGenFunction::StartFunction(…
…
DI->setLocation(StartLoc);
DI->EmitFunctionStart(GD, FnType, CurFn, Builder);
}

– adrian

8

Hi David,

I committed the improved version of the test in r201248.

– adrian

9

Hi David,

I committed the improved version of the test in r201248.

Thanks for that - sorry for the disruption/inconvenience.