Add debug support for set types
Needs ReviewPublic
Actions

Authored by demoitem on Mar 12 2020, 9:12 PM.

Details

Reviewers

probinson
echristo
dblaikie
aprantl

Summary

This commit adds debugging support for set types defined in languages such as pascal and modula-2

Diff Detail

Unit TestsFailed
View All

	Time	Test
	330 ms	x64 debian > LLVM.DebugInfo/Generic::set.ll Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llc -debugger-tune=gdb -dwarf-version=4 -filetype=obj -o /mnt/disks/ssd0/agent/llvm-project/build/test/DebugInfo/Generic/Output/set.ll.tmp.o < /mnt/disks/ssd0/agent/llvm-project/llvm/test/DebugInfo/Generic/set.ll
	0 ms	x64 debian > LLVM.Verifier::set1.ll Test has no 'RUN:' line
	140 ms	x64 windows > LLVM.DebugInfo/Generic::set.ll Script: -- : 'RUN: at line 3'; c:\ws\w1\llvm-project\premerge-checks\build\bin\llc.exe -debugger-tune=gdb -dwarf-version=4 -filetype=obj -o C:\ws\w1\llvm-project\premerge-checks\build\test\DebugInfo\Generic\Output\set.ll.tmp.o < C:\ws\w1\llvm-project\premerge-checks\llvm\test\DebugInfo\Generic\set.ll
	0 ms	x64 windows > LLVM.Verifier::set1.ll Test has no 'RUN:' line

Event Timeline

demoitem created this revision.Mar 12 2020, 9:12 PM

Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2020, 9:12 PM

Herald added subscribers: llvm-commits, hiraditya. · View Herald Transcript

Harbormaster failed remote builds in B49092: Diff 250121!Mar 12 2020, 9:54 PM

dblaikie added a reviewer: dblaikie.Mar 22 2020, 7:38 PM

dblaikie added a project: debug-info.

Seems reasonable to me - thanks!

This revision is now accepted and ready to land.Mar 23 2020, 1:37 PM

In your example you're encoding set as a derived type over an enum. That seems like a fine approach to me, but I think the interface should enforce this. Perhaps the Verifier should reject set derived typed from anything but an enum and the DIBuilder should only accept DICompositeTypes as the base type? Are there non-enum set types that you also want to encode?
Does this also need dwarfdump support, or does this work already?

Added extra Verifier checks to ensure the base type of a set is either an Enumeration or a Base type with integer encoding.
I will extend this check when I land a patch to extend subranges since a set can have a subrange as a basetype.
The llvm-dwarfdump tool outputs set debugging information without any enhancements.
I do not have commit access, so I wonder if someone could commit these changes if acceptable?

Could someone please commit this patch if acceptable.

Looks like you accidentally just updated the diff on top of your previous patch?
Is the Verifier change being tested?

I'm new to this, my first contribution, so bear with me.

On my local branch I had 2 commits
and I used the following to get a patch (following the docs)

git show HEAD -U999999 > set.patch

The second patch I uploaded seemed to only have the changes from the second commit - which I thought was okay.

Can you advise? Is each patch supposed to contain the complete set of commits for the changed code and get a new Differential number?

I am slightly confused.

Also, regards the Verifier changes, I have only tested them locally but I guess I should produce a test file. So I will get started on that.

Thanks

In D76115#2001144, @demoitem wrote:

I'm new to this, my first contribution, so bear with me.

On my local branch I had 2 commits
and I used the following to get a patch (following the docs)

git show HEAD -U999999 > set.patch

The HEAD refers to the SHA of the specific commit.

The second patch I uploaded seemed to only have the changes from the second commit - which I thought was okay.

Can you advise? Is each patch supposed to contain the complete set of commits for the changed code and get a new Differential number?

Yes. If the patches are logically different it makes sense to make separate changes here. If the diffs are connected (they represent the same functionality/feature/intention), it makes sense to make a stack of the patches (Edit Related Revision -> Edit Parent/Child rev).

So, if your local repo looks as:

commit 878787...
Author: ...
Date:   ...

    [LLVM] ..

commit 989898...
Author: ...
Date:   ...

    [Clang] ..

you can create the patches for the Phabricator as:

git show 989898... -U999999 > name_of_the_change1.patch
git show 878787... -U999999 > name_of_the_change2.patch

I am slightly confused.

Also, regards the Verifier changes, I have only tested them locally but I guess I should produce a test file. So I will get started on that.

Thanks

Added a new test

Herald added a subscriber: dexonsmith. · View Herald TranscriptDec 10 2020, 4:17 PM

I know this change is old but I'd like to get it committed if possible. (Got waylaid by the pandemic) Or maybe abandon it and resubmit?
Anyway, are there any reviewers out there?

I think this looks mostly good, but it looks like it's missing a unittest for the DIBuilder function that you added.

You wouldn't happen to know a similar unit test that I could base it on?

I checked out the unit test directory and could not see anything useful.

In D76115#2537948, @demoitem wrote:

You wouldn't happen to know a similar unit test that I could base it on?

I checked out the unit test directory and could not see anything useful.

Did you look in unittests/IR/DebugInfoTest.cpp (e.g., at CreateFortranArrayTypeWithAttributes)?