Page MenuHomePhabricator

[DWARF-5] Support for DWARF-5 C++ language tags
ClosedPublic

Authored by SouraVX on Sep 16 2019, 5:17 AM.

Diff Detail

Repository
rC Clang

Event Timeline

SouraVX created this revision.Sep 16 2019, 5:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2019, 5:17 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aprantl added inline comments.Sep 16 2019, 9:56 AM
include/clang/AST/DeclCXX.h
2931

I understand that DWARF does not define a C++17 language constant, but in the AST I fell like we should represent it, even if it is being lowered into C++14 for the debug info.

lib/AST/DeclPrinter.cpp
1009

Can you turn this into a switch() statement?

1010

Why this weird spelling with the underscore?

SouraVX marked 3 inline comments as done.Sep 16 2019, 11:22 PM
SouraVX added inline comments.
include/clang/AST/DeclCXX.h
2931

It's represented in AST as enum in LangStandard.h file. We didn't add it here, as DWARF5 has no opcode for C++17. If we add it here some opcode, this might cause conflict with future upcoming DWARF standard.

lib/AST/DeclPrinter.cpp
1009

Done

SouraVX updated this revision to Diff 220435.Sep 16 2019, 11:29 PM
SouraVX updated this revision to Diff 220472.Sep 17 2019, 4:36 AM
SouraVX set the repository for this revision to rC Clang.

Removed underscores from names.

aprantl added inline comments.Sep 17 2019, 8:26 AM
include/clang/AST/DeclCXX.h
2931

Since these are the DWARF DW_lang constants, we shouldn't repeat their numeric values here. Can we write this as
lang_c = llvm::dwarf::DW_LANG_C, etc?

SouraVX marked an inline comment as done.Sep 17 2019, 9:19 AM
SouraVX added inline comments.
include/clang/AST/DeclCXX.h
2931

We can do this, but not sure whether, we should do this in AST ?
Since it's a language specific part, if we integrate this from DWARF opcodes it will unnecessary pollute AST ? Other language opcodes will also be visible here.

SouraVX marked an inline comment as not done.Sep 17 2019, 9:37 AM
aprantl added inline comments.Sep 17 2019, 10:01 AM
include/clang/AST/DeclCXX.h
2931

I'm not sure I understand the concern here. Are you worried about #Including "llvm/BinaryFormat/DWARF.h" and making these definitions visible to users of DeclCXX.h? All symbols there are in a separate namespace.

SouraVX marked 2 inline comments as done.Sep 17 2019, 10:49 AM
SouraVX added inline comments.
include/clang/AST/DeclCXX.h
2931

Thanks @aprantl for your review.
I was sceptical about, introducing DWARF related changes to AST, however you're correct, those are just language enums.
This will be heplfull in future, while adding language tag. If more C/C++ versions become part of DWARF standard.
Will address this change in next revision.

SouraVX marked an inline comment as done.Sep 17 2019, 10:50 AM
SouraVX updated this revision to Diff 220613.Sep 17 2019, 10:35 PM

Updated AST to use DWARF language enums.

aprantl accepted this revision.Sep 18 2019, 2:24 PM

Please be sure the watch the lldb (and other debugger) bots after committing this..

This revision is now accepted and ready to land.Sep 18 2019, 2:24 PM

Please be sure the watch the lldb (and other debugger) bots after committing this..

Thanks a lot!

Please be sure the watch the lldb (and other debugger) bots after committing this..

Thanks a lot!

@aprantl , Could you commit these changes, I don't have commit access rights.
Thanks!

Please be sure the watch the lldb (and other debugger) bots after committing this..

Could you please commit these changes.
Thanks!

Closed by commit rL372663: Support for DWARF-5 C++ language tags. (authored by adrian, committed by ). · Explain WhySep 23 2019, 3:00 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2019, 3:00 PM

/srv/llvm-buildbot-srcatch/llvm-build-dir/clang-x86_64-debian-fast/llvm.src/tools/clang/tools/extra/modularize/Modularize.cpp:583:13: warning: enumeration values 'lang_cxx_11' and 'lang_cxx_14' not handled in switch [-Wswitch]
1 warning generated.

I reverted this in r372672 because it caused a test failure in LLDB: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/1748/

uabelho added a subscriber: uabelho.EditedSep 23 2019, 11:25 PM

/srv/llvm-buildbot-srcatch/llvm-build-dir/clang-x86_64-debian-fast/llvm.src/tools/clang/tools/extra/modularize/Modularize.cpp:583:13: warning: enumeration values 'lang_cxx_11' and 'lang_cxx_14' not handled in switch [-Wswitch]
1 warning generated.

We also get that warning when compiling with clang 8:

../../clang-tools-extra/modularize/Modularize.cpp:583:13: error: enumeration values 'lang_cxx_11' and 'lang_cxx_14' not handled in switch [-Werror,-Wswitch]
    switch (D->getLanguage()) {
            ^
1 error generated.

/srv/llvm-buildbot-srcatch/llvm-build-dir/clang-x86_64-debian-fast/llvm.src/tools/clang/tools/extra/modularize/Modularize.cpp:583:13: warning: enumeration values 'lang_cxx_11' and 'lang_cxx_14' not handled in switch [-Wswitch]
1 warning generated.

We also get that warning when compiling with clang 8:

../../clang-tools-extra/modularize/Modularize.cpp:583:13: error: enumeration values 'lang_cxx_11' and 'lang_cxx_14' not handled in switch [-Werror,-Wswitch]
    switch (D->getLanguage()) {
            ^
1 error generated.

I did something about it in r372714. Please take a look and see if it should be fixed differently.

/srv/llvm-buildbot-srcatch/llvm-build-dir/clang-x86_64-debian-fast/llvm.src/tools/clang/tools/extra/modularize/Modularize.cpp:583:13: warning: enumeration values 'lang_cxx_11' and 'lang_cxx_14' not handled in switch [-Wswitch]
1 warning generated.

We also get that warning when compiling with clang 8:

../../clang-tools-extra/modularize/Modularize.cpp:583:13: error: enumeration values 'lang_cxx_11' and 'lang_cxx_14' not handled in switch [-Werror,-Wswitch]
    switch (D->getLanguage()) {
            ^
1 error generated.

/srv/llvm-buildbot-srcatch/llvm-build-dir/clang-x86_64-debian-fast/llvm.src/tools/clang/tools/extra/modularize/Modularize.cpp:583:13: warning: enumeration values 'lang_cxx_11' and 'lang_cxx_14' not handled in switch [-Wswitch]
1 warning generated.

We also get that warning when compiling with clang 8:

../../clang-tools-extra/modularize/Modularize.cpp:583:13: error: enumeration values 'lang_cxx_11' and 'lang_cxx_14' not handled in switch [-Werror,-Wswitch]
    switch (D->getLanguage()) {
            ^
1 error generated.

I did something about it in r372714. Please take a look and see if it should be fixed differently.

Hi Thanks for doing this, I got occupied by something else. that seems good.
Thanks again!