Page MenuHomePhabricator

[Debug-Info] DW_AT_export_symbols shouldn't be generated before version-5 of DWARF.
AbandonedPublic

Authored by Esme on Apr 13 2021, 8:25 PM.

Details

Reviewers
shchenz
aprantl
dblaikie
jsji
Group Reviewers
Restricted Project
Summary

DW_AT_export_symbols is an attribute introduced in DWARF-5, which should not be generated under previous DWARF versions. And this unexpected behavior caused the DBX to fail.

Diff Detail

Event Timeline

Esme created this revision.Apr 13 2021, 8:25 PM
Esme requested review of this revision.Apr 13 2021, 8:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2021, 8:25 PM

@aprantl @probinson @jhenderson - any of you folks want this in older versions? I think this is for C++ inline namespaces - so if you used a modern C++ standard library (that uses inline namespaces like "std::__1::basic_string", etc) users would have to use these extra qualifiers when naming types, functions, etc. Not unworkable, but a bit inconvenient.

shchenz added a comment.EditedApr 16 2021, 3:18 AM

Can we guard the fix only for DBX? So that other consumers will not be impacted?

Esme updated this revision to Diff 338082.Apr 16 2021, 6:18 AM

Addressed Zheng's comments.

@aprantl @probinson @jhenderson - any of you folks want this in older versions? I think this is for C++ inline namespaces - so if you used a modern C++ standard library (that uses inline namespaces like "std::__1::basic_string", etc) users would have to use these extra qualifiers when naming types, functions, etc. Not unworkable, but a bit inconvenient.

I'm not the right person to ask, but I wouldn't expect a DWARF v4 consumer to be able to recognise a DWARF v5 attribute in general, and therefore we shouldn't be using emitting it in older cases. @probinson or maybe @jmorse will probably have a better insight here.

@aprantl @probinson @jhenderson - any of you folks want this in older versions? I think this is for C++ inline namespaces - so if you used a modern C++ standard library (that uses inline namespaces like "std::__1::basic_string", etc) users would have to use these extra qualifiers when naming types, functions, etc. Not unworkable, but a bit inconvenient.

That's right. LLDB is using this attribute even when parsing DWARF v4 to model inline namespaces (such as libc++'s __1 namespace). If we don't have the attribute then we end up with a bunch of __1 in the type names we show to the user (+ some other minor changes in behaviour). It shouldn't break any LLDB scripts though as the type names used in scripts always specify inline namespaces.

If we decide to remove this for all consumers we can just special-case the namespace parsing in LLDB to infer that std::__1 is an inline namespace. It's the only namespace that is important for users (and the test suite).

@aprantl @probinson @jhenderson - any of you folks want this in older versions? I think this is for C++ inline namespaces - so if you used a modern C++ standard library (that uses inline namespaces like "std::__1::basic_string", etc) users would have to use these extra qualifiers when naming types, functions, etc. Not unworkable, but a bit inconvenient.

I'm not the right person to ask, but I wouldn't expect a DWARF v4 consumer to be able to recognise a DWARF v5 attribute in general, and therefore we shouldn't be using emitting it in older cases. @probinson or maybe @jmorse will probably have a better insight here.

Generally we figure emitting new tags/attributes is OK, because consumers are meant to ignore them if they don't recognize them - and some consumers can recognize them as an extension. (such as lldb)

This conversation's gotten fragmented over a bunch of reviews, unfortunately - but it sounds like the direction it's headed is that maybe -gstrict-dwarf will be wired up to do all this pedantry/not emit things from future standards, and DBX tuning will opt in to -gstrict-dwarf mode.

@aprantl @probinson @jhenderson - any of you folks want this in older versions? I think this is for C++ inline namespaces - so if you used a modern C++ standard library (that uses inline namespaces like "std::__1::basic_string", etc) users would have to use these extra qualifiers when naming types, functions, etc. Not unworkable, but a bit inconvenient.

That's right. LLDB is using this attribute even when parsing DWARF v4 to model inline namespaces (such as libc++'s __1 namespace). If we don't have the attribute then we end up with a bunch of __1 in the type names we show to the user (+ some other minor changes in behaviour). It shouldn't break any LLDB scripts though as the type names used in scripts always specify inline namespaces.

If we decide to remove this for all consumers we can just special-case the namespace parsing in LLDB to infer that std::__1 is an inline namespace. It's the only namespace that is important for users (and the test suite).

Thanks for the context. (from other threads, sounds like we might be moving more towards DBX folks wiring up -gstrict-dwarf to implement all this stuff and enabling -gstrict-dwarf when targeting DBX by default - leaving all these future-spec-feature-in-prior-spec-mode on by default for everyone else)

shchenz added a comment.EditedTue, Apr 20, 1:40 AM

@aprantl @probinson @jhenderson - any of you folks want this in older versions? I think this is for C++ inline namespaces - so if you used a modern C++ standard library (that uses inline namespaces like "std::__1::basic_string", etc) users would have to use these extra qualifiers when naming types, functions, etc. Not unworkable, but a bit inconvenient.

I'm not the right person to ask, but I wouldn't expect a DWARF v4 consumer to be able to recognise a DWARF v5 attribute in general, and therefore we shouldn't be using emitting it in older cases. @probinson or maybe @jmorse will probably have a better insight here.

Generally we figure emitting new tags/attributes is OK, because consumers are meant to ignore them if they don't recognize them - and some consumers can recognize them as an extension. (such as lldb)

This conversation's gotten fragmented over a bunch of reviews, unfortunately - but it sounds like the direction it's headed is that maybe -gstrict-dwarf will be wired up to do all this pedantry/not emit things from future standards, and DBX tuning will opt in to -gstrict-dwarf mode.

Thanks for the explanation @dblaikie Yes, we just add -gstrict-dwarf support in D100809 and D100826. This option is turned off by default and only enabled for DBX.

I think we also need to change this patch accordingly. @Esme

Esme updated this revision to Diff 338802.Tue, Apr 20, 3:53 AM

use strict dwarf flag instead of DBX debugger

There may be an opportunity to do this more robustly, instead of scattering the checks all over the place. Note that the Dwarf.def file already knows the version for each tag and attribute; DwarfDebug already knows the version we're emitting; so if DwarfDebug also had the strict-dwarf flag, it would be easy to add a helper predicate or two that would do the checking for any tag or attribute.

Then the methods that add attributes could generally call the predicate themselves, instead of having higher-level code do it.
For tags you probably do want the higher-level code to call the predicate, but it seems like it would be a lot simpler to do something like if (canEmitTag(dwarf::DW_TAG_rvalue_reference)) than the kind of thing we're seeing in these patches.

Does that seem reasonable?

There may be an opportunity to do this more robustly, instead of scattering the checks all over the place. Note that the Dwarf.def file already knows the version for each tag and attribute; DwarfDebug already knows the version we're emitting; so if DwarfDebug also had the strict-dwarf flag, it would be easy to add a helper predicate or two that would do the checking for any tag or attribute.

Then the methods that add attributes could generally call the predicate themselves, instead of having higher-level code do it.
For tags you probably do want the higher-level code to call the predicate, but it seems like it would be a lot simpler to do something like if (canEmitTag(dwarf::DW_TAG_rvalue_reference)) than the kind of thing we're seeing in these patches.

Does that seem reasonable?

Agreed - brought up something similar here: https://reviews.llvm.org/D100826#2702135

(For the tags, yeah, I'm not sure - wonder if the code for emitting the DIEs could return null/empty DIE/something to indicate that the DIE wasn't created - though that might be hard to retrofit if it means revisiting every place that constructs a DIE to ensure it can handle a "nothing" return)

Esme abandoned this revision.Sun, Apr 25, 6:41 PM