Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

backend: dwarfdbginf: Remove artificial attributes on wchar_t typedef #13247

Merged
merged 1 commit into from Nov 5, 2021

Conversation

ljmf00
Copy link
Member

@ljmf00 ljmf00 commented Oct 31, 2021

According to DWARF, declaration coordinates are not mandatory and wchar_t is
currently an artificial typedef. We should not generate DECL attributes for
such artificial TAGs.

Signed-off-by: Luís Ferreira contact@lsferreira.net

@dlang-bot
Copy link
Contributor

dlang-bot commented Oct 31, 2021

Thanks for your pull request, @ljmf00!

Bugzilla references

Auto-close Bugzilla Severity Description
22467 normal DWARF: wchar_t reports wrong DECL attributes

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#13247"

@Geod24
Copy link
Member

Geod24 commented Oct 31, 2021

Sounds right. I'll have a look at the spec just in case. Got any ref for us ?

@ljmf00 ljmf00 marked this pull request as draft October 31, 2021 23:36
@ljmf00
Copy link
Member Author

ljmf00 commented Oct 31, 2021

Sounds right. I'll have a look at the spec just in case. Got any ref for us ?

I wonder why this is necessary. This doesn't work particularly with typedef. I'm going to file a bug on the upstream of LLDB and GDB as both don't recognize this correctly.

I'm surprised that only LLDB hangs on this, although llvm-dwarfdump validates the file as valid DWARF TAGs. And with hang I mean, it freezes without even catching signals. GDB simply fails at showing the variables.

According to the specification in 2.14 Declaration Coordinates:

Any debugging information entry representing the declaration of an object,
module, subprogram or type may have DW_AT_decl_file, DW_AT_decl_line and
DW_AT_decl_column attributes, each of whose value is an unsigned integer
constant.

I interpret may word as what is followed by 1.3.12 Permissive Rather Than Prescriptive in the introduction chapter:

In this document, where the word “may” is used, the producer has the option to
follow the description or not

For now, I'm going to change this to match the C/C++ behaviour. wchar_t is supposed to be a native type. This is how clang handles it:

0x000000c2:   DW_TAG_base_type
                DW_AT_name	("wchar_t")
                DW_AT_encoding	(DW_ATE_signed)
                DW_AT_byte_size	(0x04)

We shouldn't use this type for D anyway, since wchar_t doesn't match with the specification of our wchar, plus, our wchar is supposed to be DW_ATE_UTF. On that note, I'm going to fix this to DW_TAG_base_type and make a separate PR to handle this correctly for D.

debug_info.buf.write16(1); // DW_AT_decl_line
typidx_tab[ty] = idx;
debug_info.buf.writeByte(tysize(TYint)); // DW_AT_byte_size
debug_info.buf.writeByte(DW_ATE_signed); // DW_AT_encoding
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is technically signed because it mimics the int type. This behaviour complies with clang/gcc debug info.

@ljmf00
Copy link
Member Author

ljmf00 commented Nov 1, 2021

Since this logic belongs to DMC I'm not going to test this here, unless we have some test suite for SCPP version. I'm going to stack a PR on top of this to implement whar for D.

According to DWARF, declaration coordinates are not mandatory and wchar_t is
currently an artificial typedef. We should not generate DECL attributes for
such artificial TAGs.

Fixes #22467 .

Signed-off-by: Luís Ferreira <contact@lsferreira.net>
@ljmf00 ljmf00 marked this pull request as ready for review November 1, 2021 19:12
@ljmf00 ljmf00 requested a review from Geod24 November 1, 2021 19:13
@ljmf00
Copy link
Member Author

ljmf00 commented Nov 1, 2021

Can you re-review @Geod24 ?

@RazvanN7 RazvanN7 added the 72h no objection -> merge The PR will be merged if there are no objections raised. label Nov 2, 2021
@RazvanN7 RazvanN7 merged commit abd3c92 into dlang:master Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
72h no objection -> merge The PR will be merged if there are no objections raised. Bug Fix
Projects
None yet
4 participants