[DwarfGenerator] Calculate relative offset according to Dwarf Version

[hawkinsw]

The relative offset for a CU in Dwarf v5 (and later) is different than the relative offset for a CU in Dwarf v4 (and before).

[hawkinsw]

Spent half a day tracking down why my code based on DwarfGenerator was not doing the right thing before I realized that 11 and 12 are different numbers. I know it's a small change, but I hope that it helps!

Thank you for all that you do to keep LLVM going!

[clayborg]

Looks good. Can we add a unit test for this?

[hawkinsw]

Looks good. Can we add a unit test for this?

I'm on it!!

[jh7370]

LGTM, not sure how this isn't already causing issues...!

Looks good. Can we add a unit test for this?

This is already code for generating test inputs and nothing else. What exactly would you test?

[hawkinsw]

LGTM, not sure how this isn't already causing issues...!

I had exactly the same thought. The reason there is no breakage, I think, is that it "mostly" works and if the unit tests are "round tripping" (ie, emit and read), it is "close enough" that it works. In particular, if you use llvm-dwarfdump on the emitted blobs, you get "mostly" good information.

But, seeing this code made me have one of those classic programmer moments: "How did this ever work?"

Looks good. Can we add a unit test for this?

This is already code for generating test inputs and nothing else. What exactly would you test?

I also wondered that, but was/am willing to brainstorm and have a try at it!

Thank you for the review!!

[hawkinsw]

LGTM, not sure how this isn't already causing issues...!

I had exactly the same thought. The reason there is no breakage, I think, is that it "mostly" works and if the unit tests are "round tripping" (ie, emit and read), it is "close enough" that it works. In particular, if you use llvm-dwarfdump on the emitted blobs, you get "mostly" good information.

But, seeing this code made me have one of those classic programmer moments: "How did this ever work?"

Looks good. Can we add a unit test for this?

This is already code for generating test inputs and nothing else. What exactly would you test?

I also wondered that, but was/am willing to brainstorm and have a try at it!

Thank you for the review!!

I just wanted to touch base and see whether we want to try to find a way to make a unit test for this change, @clayborg and @jh7370 ? As I said above, I am more than happy to give it a try but I didn't want to do something without asking!

Will

[jh7370]

LGTM, not sure how this isn't already causing issues...!

I had exactly the same thought. The reason there is no breakage, I think, is that it "mostly" works and if the unit tests are "round tripping" (ie, emit and read), it is "close enough" that it works. In particular, if you use llvm-dwarfdump on the emitted blobs, you get "mostly" good information.
But, seeing this code made me have one of those classic programmer moments: "How did this ever work?"

Looks good. Can we add a unit test for this?

This is already code for generating test inputs and nothing else. What exactly would you test?

I also wondered that, but was/am willing to brainstorm and have a try at it!
Thank you for the review!!

I just wanted to touch base and see whether we want to try to find a way to make a unit test for this change, @clayborg and @jh7370 ? As I said above, I am more than happy to give it a try but I didn't want to do something without asking!

Will

I'm not convinced we should be adding unit tests to test that test helper code is doing the right thing. In theory, at least, there should be tests of real code (i.e. not test code) that make use of each piece of functionality of this code. In this case, the functionality is not really being used by any existing tests - it might be used incidentally, but the behaviour of that functionality must be irrelevant or either the tests would be failing or the code is broken in some way and the test helper code bug is hiding that breakage.

I guess what I'm thinking is, the tests that use the generator are the tests for the generator. It sounds to me like @hawkinsw specifically is writing tests that fail because of the bug in the generator, so those tests become the tests that cover this (assuming these tests are currently in or going to be in upstream LLVM somewhere anyway). This works because unless there are bugs in both the real code and the generator code that cancel each other out, a test will fail. If there are bugs in both, there's not much we can do about it, but this is tangential - we can't go and write a third implementation just to check that our other two are both correct (well, we could, but then we'd need a fourth implementation to test that, which in turn needs a fifth etc).

[hawkinsw]

First, I am terribly sorry for not seeing your response sooner!

LGTM, not sure how this isn't already causing issues...!

I had exactly the same thought. The reason there is no breakage, I think, is that it "mostly" works and if the unit tests are "round tripping" (ie, emit and read), it is "close enough" that it works. In particular, if you use llvm-dwarfdump on the emitted blobs, you get "mostly" good information.
But, seeing this code made me have one of those classic programmer moments: "How did this ever work?"

Looks good. Can we add a unit test for this?

This is already code for generating test inputs and nothing else. What exactly would you test?

I also wondered that, but was/am willing to brainstorm and have a try at it!
Thank you for the review!!

I just wanted to touch base and see whether we want to try to find a way to make a unit test for this change, @clayborg and @jh7370 ? As I said above, I am more than happy to give it a try but I didn't want to do something without asking!
Will

I'm not convinced we should be adding unit tests to test that test helper code is doing the right thing. In theory, at least, there should be tests of real code (i.e. not test code) that make use of each piece of functionality of this code. In this case, the functionality is not really being used by any existing tests - it might be used incidentally, but the behaviour of that functionality must be irrelevant or either the tests would be failing or the code is broken in some way and the test helper code bug is hiding that breakage.

I guess what I'm thinking is, the tests that use the generator are the tests for the generator. It sounds to me like @hawkinsw specifically is writing tests that fail because of the bug in the generator, so those tests become the tests that cover this (assuming these tests are currently in or going to be in upstream LLVM somewhere anyway). This works because unless there are bugs in both the real code and the generator code that cancel each other out, a test will fail. If there are bugs in both, there's not much we can do about it, but this is tangential - we can't go and write a third implementation just to check that our other two are both correct (well, we could, but then we'd need a fourth implementation to test that, which in turn needs a fifth etc).

And then a sixth! Thank you for the response, @jh7370 . I agree with you. I discovered this issue while attempting to write a standalone "library" for another project that needed to emit dwarf information. In particular, I chose to use llvm's tooling because of it's support for emitting the DW_LNCT_llvm_source (or DW_LNCT_source when it is standardized). I was reading the test cases to get a sense for how to use the different pieces of code that are dwarf-related in llvm. They were very helpful, of course.

Thank you for all the work that you do on llvm and, again, I apologize for not responding sooner. Just let me know if/how I can shape up this PR so that it can be incorporated!
Will

[jh7370]

Do you have commit access? You can squash and merge this as soon as you're ready, if so. Alternatively, I can do so if you need me to.

[hawkinsw]

Do you have commit access? You can squash and merge this as soon as you're ready, if so. Alternatively, I can do so if you need me to.

I do, but I am still relatively "new" to the process and hesitant/nervous. If you would be willing to commit, I would appreciate it.

On the other hand, I have to learn sometime.

[jh7370]

Go ahead and click "Squash and merge" when you're ready. Only thing to make sure is that you haven't got your email address hidden by GitHub. Then keep an eye out for failure emails from the build bots (not all failures will necessarily be anything to do with your change though).

[hawkinsw]

Go ahead and click "Squash and merge" when you're ready. Only thing to make sure is that you haven't got your email address hidden by GitHub. Then keep an eye out for failure emails from the build bots (not all failures will necessarily be anything to do with your change though).

Thanks! I have done a few with libc++ so here. we. go.