[llvm] [DwarfGenerator] Calculate relative offset according to Dwarf Version (PR #84847)
Will Hawkins via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 18 22:35:21 PDT 2024
hawkinsw wrote:
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
https://github.com/llvm/llvm-project/pull/84847
More information about the llvm-commits
mailing list