My comments have been addressed, but leaving final approval to @dblaikie.

Some parts of this could be extracted into a separate NFC (or mostly NFC) patch:

• The Dwarf.h part
• In DWARFAcceleratorTable.h, merging the Header and HeaderPOD structs, and removing the Padding field. (Adding the Format field would still be part of the functional patch.)
• In DWARFAcceleratorTable.cpp, eliminating the Padding field, and introducing CommonHeaderSize in DWARFDebugNames::Header::extract()

In D71487#1791824, @clayborg wrote:

BTW: is used to be that both DW_AT_low_pc and DW_AT_high_pc would be set to zero when a function was dead stripped. This was back when both the low and high pc used DW_FORM_addr (a file address). But then DWARF changed such that DW_AT_high_pc could be encoded as a data form: DW_FORM_data1, DW_FORM_data2, DW_FORM_data4, or DW_FORM_data8. This is used to mean it is an offset from the low PC. Seems the linkers now didn't have a relocation for the DW_AT_high_pc so they couldn't zero it out. This is sad because we can end up with many functions at address zero that didn't get linked, and if zero is a valid address, then our DWARF contains a bunch of useless info that only hides which function is the real function for address zero.

In D69309#1787994, @friss wrote:

In D69309#1787409, @labath wrote:

In D69309#1787297, @jarin wrote:

In D69309#1752738, @friss wrote:

Basically, today the debug info will describe an entity named "Foo<int>". The accelerator tables all reference this name. So when Clang asks us if we know "Foo" (which is what happens when instantiating), we fail to find the right instantiations. The consensus of the above discussion was that we should change the debug info to have "Foo" as the name of any instantiation, with a child DIE describing the template arguments. Just doing this in the compiler causes test failures in LLDB, so there's some work to do in LLDB to support this.

Frederic, you say that "doing this in the compiler causes test failures in LLDB", which implies you have tried adding the template in the compiler. Do you have that compiler patch lying around so that we could have a look at what can be done on the lldb side?

I agree that a good long term fix is to have "Foo" as an entity in DWARF, although for backwards compatibility it might be better if the "Foo" template just contained references to the instantiations rather than having them as children.

I am afraid you're overestimating the scope of that idea. I *think* that Fred was referring to simply changing the string that gets put into the DW_AT_name field of the /instantation/ (and, by extension, the accelerator table). The debug info would still describe instantiations only.

Just confirming that this is indeed what I meant. I don't have the patch handy anymore (it was extremely small, about 5 lines IIRC).

Seems to me we've needed to do this sort of tweak before, so LGTM.
Because it depends on the memory-management behavior of the map, it would be tricky to test with IR input. Would it be feasible to do something in llvm/unittests/ADT/SCCIteratorTest.cpp that could trigger this reliably? Can be done as a follow-up, as I know the branch is coming soon.

"DW_TAG_unspecified_type auto" should be emitted for the function declared/defined as auto returnning. Do you have other test cases in mind, where above points diverges ??

@dblaikie let me reflect this back to make sure I get it:
Template members (methods or variables) would never appear in the *metadata* description of the struct; but metadata descriptions of the instances would refer back to that struct (as the scope for the instance). Then DwarfDebug would paste them all together when emitting the DWARF description(s). The in-struct child DIE could then have the deduced type because by the time the definition metadata is produced, we actually know what that type is. This is okay because template data members are necessarily static; they don't affect size or layout of the struct in any way.

In D71508#1786122, @kamleshbhalui wrote:

In D71508#1785767, @probinson wrote:

Do we have a similar problem if the filespec has an embedded ./ or ../ in it?

problems occur only when filespec starts with ./ otherwise it's fine.

I have to wonder if we're not being too eager to produce the debug info. It seems that the undeduced type problem arises because we're trying to produce debug info before we've really finished instantiating value<int> here. But how Clang works pretty much always confuses me.

LGTM

Do we have a similar problem if the filespec has an embedded ./ or ../ in it? I'm thinking some broader canonicalization ought to be done here.
$ clang ./dir1/dir2/../dir3/file.c
should resolve to dir1/dir3/file.c shouldn't it?

Hmm, maybe this feature/suggestion is broken or at least not exactly awesome when it comes to auto-returning functions that are eventually void-returning functions? Now the function definition has no DW_AT_type to override the unspecified_type in the declaration... :/ that's unfortunate (@probinson - thoughts?)

I'd rather see REQUIRES-ANY deprecated/removed, in favor of using expressions in REQUIRES. (REQUIRES-ANY predates expressions.)
IIRC it is used rarely and it would not be a big deal to eliminate it.

LGTM.

I like it, but the others should have a chance to say something.

Yep LGTM.

Address the inline comments and LGTM.

Perhaps we should implement this mode under -fno-standalone-debug (or something more aggressive) since "standalone" is one of the only places I can think of where having the full class definition would be handy

In D70524#1772117, @dblaikie wrote:

In D70524#1771709, @probinson wrote:

In D70524#1771522, @shafik wrote:

@probinson I was reading the C++ "auto" return type issue and I can't come up with a case where we don't have class descriptions across compilation units that are not consistent wrt to auto return type. Do you have a specific example?

The actual return type is known in a compile_unit where the method is defined, and not known in other compile_units. If the non-defining compile_units omit the return type, that means "void" not "auto". That is, one compile unit says it returns "void" and another compile unit says it returns something else. That is the inconsistency I meant.

If we use unspecified_type instead of omitting the return type, then a consumer knows that the method returns *something*, but it will have to look elsewhere to determine what that is.

Yeah, my argument was to omit the declarations of auto-returning functions entirely except in cases where their definition is available (either only when the definition is emitted, or whenever the definition's available so the deduced return type can be provided), same as a template instantiation.

So the class would be missing the declaration of the function in cases where the definition isn't available - same as templates and implicit special members.

In D70524#1771522, @shafik wrote:

@probinson I was reading the C++ "auto" return type issue and I can't come up with a case where we don't have class descriptions across compilation units that are not consistent wrt to auto return type. Do you have a specific example?

In D70696#1765671, @dblaikie wrote:

In D70696#1765637, @probinson wrote:

For the case:

cat ref.c
    extern int global_var;
    int main() { return global_var; }

I *do* expect to see debug info for the declaration of global_var.

FWIW I'd only expect it there with -fstandalone-debug - with -fno-standalone-debug I'd expect this code to rely on the assumption that def.c is also compiled with debug info.

In D70696#1765675, @yonghong-song wrote:

It is just strange that gcc 7.3.1 (did not test, but maybe later gcc versions as well) emits the debuginfo (encoded in dwarf)
even if the external variable is not used in the current compilation unit. Not sure what is the rationale behind it.

For the case:

cat def.c
    int global_var = 2;

def.o should have debug info for the definition of global_var.
For the case:

cat noref.c
    extern int global_var;
    int main() {}

I would not expect to see debug info for the declaration of global_var.
For the case:

cat ref.c
    extern int global_var;
    int main() { return global_var; }

I *do* expect to see debug info for the declaration of global_var.

I guess first I'm confused about why the type would be undeduced in the first place, given that it is actually instantiated.
And if undeduced is correct, wouldn't we rather emit these with DW_TAG_unspecified_type?

LGTM

I know we've had previous differences of opinion on how many combinations to test, and I'll bring it up again. I think we really need:

• only one case where the two options are the same; that demonstrates a repeated option is not an error.
• minimum cases showing the higher one gets picked: never/fail, fail/always, always/help. By transitivity we conclude the others are fine.
• only one case for the other order, showing it's not first (or last) always wins.
• only two cases of environment/command, showing it's not one or the always wins.

So a total of 7 cases, rather than the 16 you have here.

In D49466#1761156, @MaskRay wrote:

The ugly path separator pattern {{(/|\\\\)}} appears in 60+ tests. Can we teach clang and other tools to

1. accept both / and \ input

In general they do, AFAIK, although it's not feasible in cases where / is the character that introduces an option, which is common on standard Windows utilities.

In D70318#1753483, @jmorse wrote:

Adrian wrote:

aprantl added a reviewer: jmorse.

Full disclosure, Stephen is also a Sony-ite, and I believe organisation-internal approves are generally frowned on. I'm happy with the general direction and implementation of this patch though.

Hmm, @aprantl @probinson - I can't find the wording in the DWARF5 spec about the use of loclists_base and rnglists_base in split units. It's unclear if these need to be specified in the split unit, or if they're assumed to be zero (or, technically, zero + sizeof(list header))? Currently LLVM doesn't generate a rnglists_base for split DWARFv5 units (I haven't checked the history on that change - but I might've had a hand in it) & this change is going to propagate that choice & do similarly for loclists_base.

Done. Makes sense to avoid future mishaps.

Move the empty lit.cfg to Inputs; undo selecting.py change.

In D70105#1745051, @jdenny wrote:

I think it's using utils\lit\tests\lit.site.cfg, which produces the error, when it would normally use utils/lit/tests/Inputs/nonexistent/lit.cfg. Creating even an empty version of that file would probably fix this.

This patch fails on Windows when built using the Visual Studio 2017 builder. The new RUN lines in selecting.py for the "nonexistent" case will produce a diagnostic complaining about a missing 'build_mode' substitution:

$ ":" "RUN: at line 7"
$ "not" "env" "-u" "FILECHECK_OPTS" "-u" "FILECHECK_DUMP_INPUT_ON_FAILURE" "C:\Program Files (x86)\Microsoft Visual Studio\Shared\Python36_64\python.exe" "D:/Data/upstream/llvm-project/llvm\utils\lit\lit.py" "D:/Data/upstream/mbuild/utils/lit/tests\Inputs/nonexistent"
$ "FileCheck" "--check-prefix=CHECK-BAD-PATH" "D:\Data\upstream\mbuild\utils\lit\tests\selecting.py"
# command stderr:
D:\Data\upstream\mbuild\utils\lit\tests\selecting.py:9:19: error: CHECK-BAD-PATH: expected string not found in input
# CHECK-BAD-PATH: Did not disover any tests for provided path(s).
                  ^
<stdin>:1:1: note: scanning from here
lit.py: D:\Data\upstream\mbuild\utils\lit\tests\lit.site.cfg:18: fatal: unable to find 'build_mode' parameter, use '--param=build_mode=VALUE'
^
<stdin>:1:62: note: possible intended match here
lit.py: D:\Data\upstream\mbuild\utils\lit\tests\lit.site.cfg:18: fatal: unable to find 'build_mode' parameter, use '--param=build_mode=VALUE'
                                                             ^

LGTM

Please remember to provide full-context diffs.

In D69886#1736266, @dblaikie wrote:

I'd expect more than 3 patches and in this order:

Sigh, one last typo. I'm happy otherwise.

Is it tested? Intuitively I would expect DW_OP_pick to be kind of an unusual operator, unlikely to be seen in the wild.

Just a couple of typos, and I'm happy. I agree with the other reviewers on the last needed test tweaks.

The test is in the right place, now it needs to behave more like other driver tests. Sorry if it feels like I'm whaling on you, but the driver is a bit of a peculiar beast with an atypical testing mode. Taming it is harder than it looks.

In D69822#1733269, @dblaikie wrote:

In D69822#1733262, @probinson wrote:

The maze of twisty -g passages gets a new secret door. Oh well.

If you find this to be a significant complication, I'd really like to discuss it further - I know there are some twisty things (one of the worst I created when it comes to -fsplit-dwarf-inlining, -gsplit-dwarf, -gmlt combinations) - but this seems pretty simple/tidy/orthogonal to me.

In D69822#1733243, @dblaikie wrote:

In D69822#1733226, @probinson wrote:

Want to decouple setting the DWARF version from enabling/disabling generation of debug info.

Um, why?

If you've got a big build system with various users opting in/out of DWARF and you want to migrate to DWARFv5, say, but you can't add "-gdwarf-5" to your build system, because that'd turn on debug info in cases that don't need it - but it's easier to change the default than to modify all cases that enable dwarf across the codebase.

Open to discussion about whether this is a good/bad idea - but it'd be useful for Google at least & seemed low-cost enough to go this route.

Want to decouple setting the DWARF version from enabling/disabling generation of debug info.

In D69393#1729772, @yonghong-song wrote:

During experimenting with linux kernel codes, I found that clang does not allow address_space attribute for function pointers, specifically, in clang/lib/Sema/SemaType.cpp,

// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "A function type shall not be
// qualified by an address-space qualifier."
if (Type->isFunctionType()) {
  S.Diag(Attr.getLoc(), diag::err_attribute_address_function_type);
  Attr.setInvalid();
  return;
}

But linux kernel tries to annotate signal handling function pointer with address space to indicate it is accessing user space.

typedef __signalfn_t __user *__sighandler_t;
typedef __restorefn_t __user *__sigrestore_t;

Such attribute makes sense for linux since indeed the signal handler code resides in user space and the kernel pointer
merely points to user memory here.

But such attributes are not allowed for function pointers.

Maybe somebody can give some context about this particular ISO/IEC TR 18037 specification? cc @probinson

In D69393#1720816, @ast wrote:

The address spaces envisioned by the Linux kernel appear to be more informational and not descriptive of hardware characteristics.

From the kernel pov the __user and normal are two different address spaces that must be accessed via different kernel primitives.
User access needs stac/clac on x86 and other precautions.
iirc other architectures have co-processor address space that needs its own load/store equivalents.
__percpu is also different address space. It's roughly equivalent to __thread in user space.

The DW_AT_address_class attribute is intended to be used for target architectures where a simple address value is ambiguous, and the debugger needs additional information to resolve the ambiguity. The canonical example is something like i386 with its segmented addresses, and an address value has different interpretations depending on whether it is (for example) a "near" or "far" pointer.
This would be in contrast to something like the VAX, where the upper two bits of the address identify a subdivision of the flat address space (VAX calls these P0, P1, S0, where P0/P1 are Process [user] spaces and S0 is system/kernel space); however, there is no ambiguity because addresses are always 32-bit and the same address value can't be interpreted different ways.

These experiments are convincing me that, in general, line zero isn't that helpful for DWARF consumers. If the goal is to get smooth stepping, we may want to refocus on getting reliable is_stmt bits in the line table.

In D68117#1711714, @dblaikie wrote:

In D68117#1711154, @probinson wrote:

(@dblaikie Aside regarding noreturn, the original DWARF proposal was for a debugger wanting to know a given function will not return.

I'd still be curious to know which consumer, if they're still interested, what feature they're trying to build with it, if they're using Clang/LLVM's output, etc.

In D68117#1710295, @dblaikie wrote:

I think the existing feature's perhaps a bit misspecified - because where you put the DEFAULTED_{in_class,out_of_class} would communicate that without needing the two values - if you put it on an out of line function definition, it's defaulted out of line, if you put it on the in-class declaration then it's defaulted inline.

But spec'd the way it is, if we want to implement this at all/if there's some actual user need (admittedly the noreturn attribute has gone in recently without a discussion of usage... so I'm not the only gatekeeper here & other people have other opinions, and that's OK - if someone (@probinson @aprantl etc) want to tell me I'm being unreasonable here & we should just put it in - it's only a bit here or there & not likely to make a huge difference to DWARF size & possibly enable some scenarios we haven't imagined yet and avoid the chicken-and-egg problem for the future implementer of such features, that's OK) - I'd essentially implement it that way. Put DEFAULTED_out_of_class on the member function definition DIE if it's defaulted there, and put DEFAULTED_in_class on the declaration DIE if it's defaulted there.

And I'd probably only spend one bit of flags on this, if possible - "not defaulted (0/assumed for all subprograms) or defaulted (1)" and translate it into one of the two values (in_class or out_of_class) in the backend based on which subprogram DIE it's attached to.

Just to clarify things in my own mind: dbg.declare expressions describe a location, while dbg.value expressions describe a value? And that's why you're wanting to add a trailing deref?
Then when we emit the actual DWARF, the trailing deref is omitted so the expression again describes a location.

Apologies for missing this until now. Our email system keeps dropping stuff sent by Phabricator.

In D68117#1709557, @SouraVX wrote:

Hi David,
I did some digging about DW_AT_defaulted and stuff, not much of a success but. Here's what I found -- http://dwarfstd.org/Issues.php?type=closed4 -- here it;s still marked as open, not sure what that means. Abbreviations on this page doesn't describe what "open" meant. But since, it's already in DWARF5 Spec -- it must be accepted.

We really do want to pack the four mutually exclusive cases into two bits. I have tried to give more explicit comments inline to explain how you would do this. It really should work fine, recognizing that the "not defaulted" case is not explicitly represented in the textual IR because it uses a zero value in the defaulted/deleted subfield of SPFlags.

I'll ask re Sony debugger. I have no direct visibility to that code.

Do we care whether llvm-dwarfdump's output bears any similarities to the output from GNU readelf or objdump? There has been a push lately to get the LLVM "binutils" to behave more like GNU's, although AFAIK it hasn't gotten to the DWARF dumping part.

For some reason a previous comment caused it to set Accept, and the only way I know to undo it is to set Request Changes. Sorry about that.

BTW the BinaryFormat part LGTM and can go in on its own if you like. Should have been done that way in the first place.

Anyone know whether they have consumers (LLDB, the Sony debugger) that would need to be updated for either the v4 changes (use of base address specifiers in classic debug_loc lists) or v5 (base_addressx, etc, etc)?

In D68465#1698682, @dblaikie wrote:

In D68465#1697824, @probinson wrote:

@dblaikie I'm also not clear what you're suggestion about .debug_addr entry plus offset. DW_LLE_offset_pair does this, derived from the base address, which ought to be available for any given function, assuming DWARF v5. Can you explain more clearly what's missing?

Right - for loclists there's no need for new forms, etc. It was specifically related to the other review related to this that modifies the in-memory representation of the debug_addr (llvm::AddressPool) - which I assume meant a difference in output in the address pool, but seems it doesn't add the offset inside the pool (but may end up with redundant entries in the pool which should be fixed in any case).

Debugger tuning should not be used directly this way. There should be a DwarfDebug flag, and a CL option, and the default set in the DwarfDebug ctor based on tuning. This allows the defaulting to work how you want, but can be overridden easily for experimentation and testing. There are lots of examples of doing this in the ctor already. Also, if it turns out some other debugger also needs this, it's trivial to fix up the ctor to handle it with no code changes needed elsewhere.

I'm happy to be wrong about the win32 part. And eliminating 'target-windows' in favor of using just 'windows' SGTM.

In D68425#1694675, @thopre wrote:

Ok to commit this revised change? I double checked that I ran the unit tests this time and all tests pass.

In D68135#1694106, @mstorsjo wrote:

Updated, with a slightly different form of the regex, that also allows the triple to just end at -windows.

Alternatively, we could just remove this part of the lit config altogether. There's no other OSes that have target-<os> as a feature (only system-<os> for the host where the test is running), and after D68133 and D68136, no tests actually use this feature any longer.

Ping

HasAddressTaken now defaults to true; this means explicitly handling innocuous instructions.

• load does not store anything.
• atomicrmw cannot store a pointer unless it goes through ptrtoint, which we handle explicitly.
• ret is irrelevant because the current frame goes away after the ret.