Page MenuHomePhabricator

[Debug-Info] strict dwarf for DW_LANG_C_plus_plus_14
ClosedPublic

Authored by shchenz on Jun 15 2021, 4:27 AM.

Details

Summary

This partially reverts the change in https://reviews.llvm.org/D99250.
Now we also generate DW_LANG_C_plus_plus_14 or DW_LANG_C_plus_plus_11 for non-strict mode.

We can provide more accurate language info for consumers which does not require strict DWARF.

Diff Detail

Event Timeline

shchenz requested review of this revision.Jun 15 2021, 4:27 AM
shchenz created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2021, 4:27 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This looks good to me.

@jzzheng22 informs me there was a comment in D99250 to the effect that DW_LANG_C_plus_plus_03 is not emitted, at all - it too was introduced in DWARF 5. I wonder if this should be addressed in a separate commit?

As mentioned in D104118, there are language codes of DW_LANG_C_plus_plus_17 and DW_LANG_C_plus_plus_20 that will be introduced in DWARF 6, which it would be good to use in non-strict mode. This would allow us to emit the more proper code of DW_LANG_C_plus_plus_17 (instead of DW_LANG_C_plus_plus_14) for C++ for OpenCL, while we wait for DW_LANG_CPP_for_OpenCL to get added, as requested at http://dwarfstd.org/ShowIssue.php?issue=210514.1. Should this be addressed in a third commit?

stuart accepted this revision.Jun 15 2021, 7:37 AM
This revision is now accepted and ready to land.Jun 15 2021, 7:37 AM

Thanks for review @stuart

This looks good to me.

@jzzheng22 informs me there was a comment in D99250 to the effect that DW_LANG_C_plus_plus_03 is not emitted, at all - it too was introduced in DWARF 5. I wonder if this should be addressed in a separate commit?

Yes, this was noticed when D99250 was posted. There is no CPlusPlus03 in LangOptions, so it is better not to merge DW_LANG_C_plus_plus_03 support with D99250.

As mentioned in D104118, there are language codes of DW_LANG_C_plus_plus_17 and DW_LANG_C_plus_plus_20 that will be introduced in DWARF 6, which it would be good to use in non-strict mode. This would allow us to emit the more proper code of DW_LANG_C_plus_plus_17 (instead of DW_LANG_C_plus_plus_14) for C++ for OpenCL, while we wait for DW_LANG_CPP_for_OpenCL to get added, as requested at http://dwarfstd.org/ShowIssue.php?issue=210514.1. Should this be addressed in a third commit?

Yes, we don't have DW_LANG_C_plus_plus_17 and DW_LANG_C_plus_plus_20 in clang for now. I guess this is because clang does not support DWRAF 6. DWARF 6 is not officially released? Once DWARF 6 is released and clang starts to support DWARF 6, I think we should add the support for DW_LANG_C_plus_plus_17 and DW_LANG_C_plus_plus_20 in the place that this patch changes.

This revision was landed with ongoing or failed builds.Jun 15 2021, 8:18 PM
This revision was automatically updated to reflect the committed changes.

There is no CPlusPlus03 in LangOptions, so it is better not to merge DW_LANG_C_plus_plus_03 support with D99250.

Oh - I see, c++03 is defined in LangStandards.def an alias for c++98, as the former essentially consists of bugfixes for the latter. This loosely suggests to me that C++03 implementations are (likely to be / mostly?) conformant to C++98, but that C++98 implementations may not be fully conformant to C++03. Given this alias, it doesn't seem at all clear to me which of DW_LANG_C_plus_plus_98 and DW_LANG_C_plus_plus_03 would be the better choice, if both C++98 and C++03 must share a language tag... but I presume this has been discussed before. (It also doesn't seem clear whether it would be better to model "c++98" as an alias for "c++03".)

Yes, we don't have DW_LANG_C_plus_plus_17 and DW_LANG_C_plus_plus_20 in clang for now. I guess this is because clang does not support DWARF 6. DWARF 6 is not officially released? Once DWARF 6 is released and clang starts to support DWARF 6, I think we should add the support for DW_LANG_C_plus_plus_17 and DW_LANG_C_plus_plus_20 in the place that this patch changes.

New DWARF language codes are published ahead of the release of the next version of DWARF, so that they may be used by implementations without having to wait for new DWARF version.

It would therefore make sense to go ahead and add DW_LANG_C_plus_plus_17 and DW_LANG_C_plus_plus_20 now, without waiting, but only in the non-strict DWARF mode. (There would be the question of whether it would still make sense in the code to say "DwarfVersion >= 6" given that DWARF 6 would be otherwise unsupported... but I don't have a strong view on that question.)

shchenz added a comment.EditedJun 16 2021, 7:09 PM

There is no CPlusPlus03 in LangOptions, so it is better not to merge DW_LANG_C_plus_plus_03 support with D99250.

Oh - I see, c++03 is defined in LangStandards.def an alias for c++98, as the former essentially consists of bugfixes for the latter. This loosely suggests to me that C++03 implementations are (likely to be / mostly?) conformant to C++98, but that C++98 implementations may not be fully conformant to C++03. Given this alias, it doesn't seem at all clear to me which of DW_LANG_C_plus_plus_98 and DW_LANG_C_plus_plus_03 would be the better choice, if both C++98 and C++03 must share a language tag... but I presume this has been discussed before. (It also doesn't seem clear whether it would be better to model "c++98" as an alias for "c++03".)

Yes, we don't have DW_LANG_C_plus_plus_17 and DW_LANG_C_plus_plus_20 in clang for now. I guess this is because clang does not support DWARF 6. DWARF 6 is not officially released? Once DWARF 6 is released and clang starts to support DWARF 6, I think we should add the support for DW_LANG_C_plus_plus_17 and DW_LANG_C_plus_plus_20 in the place that this patch changes.

New DWARF language codes are published ahead of the release of the next version of DWARF, so that they may be used by implementations without having to wait for new DWARF version.

It would therefore make sense to go ahead and add DW_LANG_C_plus_plus_17 and DW_LANG_C_plus_plus_20 now, without waiting, but only in the non-strict DWARF mode. (There would be the question of whether it would still make sense in the code to say "DwarfVersion >= 6" given that DWARF 6 would be otherwise unsupported... but I don't have a strong view on that question.)

I think it would be strange to check DwarfVersion >= 6 if we can not set dwarf version to 6 by anyways. Maybe we can first add the DWARF 6 support to the front end first(Need a wider discussion). This is the patch where -gdwarf-5 was introduced https://reviews.llvm.org/rG3cb592d6b60c. Seems it was also before DWARF 5 was official released.

There is no CPlusPlus03 in LangOptions, so it is better not to merge DW_LANG_C_plus_plus_03 support with D99250.

Oh - I see, c++03 is defined in LangStandards.def an alias for c++98, as the former essentially consists of bugfixes for the latter. This loosely suggests to me that C++03 implementations are (likely to be / mostly?) conformant to C++98, but that C++98 implementations may not be fully conformant to C++03. Given this alias, it doesn't seem at all clear to me which of DW_LANG_C_plus_plus_98 and DW_LANG_C_plus_plus_03 would be the better choice, if both C++98 and C++03 must share a language tag... but I presume this has been discussed before. (It also doesn't seem clear whether it would be better to model "c++98" as an alias for "c++03".)

Yes, we don't have DW_LANG_C_plus_plus_17 and DW_LANG_C_plus_plus_20 in clang for now. I guess this is because clang does not support DWARF 6. DWARF 6 is not officially released? Once DWARF 6 is released and clang starts to support DWARF 6, I think we should add the support for DW_LANG_C_plus_plus_17 and DW_LANG_C_plus_plus_20 in the place that this patch changes.

New DWARF language codes are published ahead of the release of the next version of DWARF, so that they may be used by implementations without having to wait for new DWARF version.

It would therefore make sense to go ahead and add DW_LANG_C_plus_plus_17 and DW_LANG_C_plus_plus_20 now, without waiting, but only in the non-strict DWARF mode. (There would be the question of whether it would still make sense in the code to say "DwarfVersion >= 6" given that DWARF 6 would be otherwise unsupported... but I don't have a strong view on that question.)

I think it would be strange to check DwarfVersion >= 6 if we can not set dwarf version to 6 by anyways. Maybe we can first add the DWARF 6 support to the front end first(Need a wider discussion). This is the patch where -gdwarf-5 was introduced https://reviews.llvm.org/rG3cb592d6b60c. Seems it was also before DWARF 5 was official released.

Yeah, sounds alright to me. Certainly if we add the version check, there should be test coverage for it - and that can only be done with the flag support for it. (& it's understandable/expected that the flag is not complete from day 1 - same thing is true of language versions (though they get speculative version numbering because the number isn't known until it's released (because it's based on the year) - but even then, once known/added, it still may not be complete because features may not have been implemented yet))