Page MenuHomePhabricator

[MC] Upgrade DWARF version to 5 upon .file 0
ClosedPublic

Authored by MaskRay on Jan 17 2021, 10:40 AM.

Details

Summary

Without -dwarf-version, llvm-mc uses the default MCContext::DwarfVersion 4.

Without -gdwarf-N, Clang cc1as uses clang::driver::ToolChain::GetDefaultDwarfVersion
which is 4 on many toolchains. Note: clang -c can synthesize .debug_info without -g.

There is currently a MCParser warning upon .file 0 and MCParser errors upon
.loc 0 if the DWARF version is less than 5. This causes friction to the
following usage:

clang -S -g -gdwarf-5 a.c

// MC warning due to .file 0, MC error due to .loc 0
clang -c a.s
llvm-mc -filetype=obj a.s

My idea is that we can just upgrade MCContext::DwarfVersion to 5 upon
.file 0 to make the above commands work.

The downside is that for an explicit version clang -c -gdwarf-4 a.s, it can be
argued that the new behavior drops the probably intended diagnostic. I think the
downside is small because in most cases DWARF version for an assembly action
should either match the original compile action or be omitted.

Ongoing discussion taking a similar action for GNU as: https://sourceware.org/pipermail/binutils/2021-January/114980.html

Diff Detail

Event Timeline

MaskRay created this revision.Jan 17 2021, 10:40 AM
MaskRay requested review of this revision.Jan 17 2021, 10:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 17 2021, 10:40 AM

Theoretically the diagnostic can be retained: we need another variable beside MCContext::DwarfVersion or making MCContext::DwarfVersion Optional<int>; and carefully doing similar thing on Clang cc1as side.

The diagnostic may not be worth the additional code complexity, though.

Maybe a silly suggestion (and one that would need more discussion on the mailing lists), but why not just change the default version to 5 more widely? How often do people deliberately deviate from the default DWARF version in practice? To me, the proposed change feels like a bit of a hack for what is likely fairly unusual usage, and is only temporary, since likely most toolchains will want to change to DWARF v5 at some point going forward.

Maybe a silly suggestion (and one that would need more discussion on the mailing lists), but why not just change the default version to 5 more widely? How often do people deliberately deviate from the default DWARF version in practice? To me, the proposed change feels like a bit of a hack for what is likely fairly unusual usage, and is only temporary, since likely most toolchains will want to change to DWARF v5 at some point going forward.

ie: you're suggesting users needing to use DWARFv4 line tables would access that functionality with a flag passed to the assembler (as they do today if they want to opt in to DWARFv5)?

I think part of the problem is that people aren't used to (ie: build systems and other infrastructure isn't designed for) having to pass a dwarf version to their assembler - the line table format hasn't changed since DWARFv2. So if you have run -gN -S, you can pass the resulting file to the compiler/assembler without any flags. So even if we switch the default - then people who aren't ready to switch to DWARFv4 are going to be in a possibly difficult situation of trying to teach their build system how to pass the appropriate flags to the assembler, etc.

I think maybe the way to think about it is that there wouldn't be a default DWARF version - it could always be determined by the presence/absence of a file 0.

Maybe a silly suggestion (and one that would need more discussion on the mailing lists), but why not just change the default version to 5 more widely? How often do people deliberately deviate from the default DWARF version in practice? To me, the proposed change feels like a bit of a hack for what is likely fairly unusual usage, and is only temporary, since likely most toolchains will want to change to DWARF v5 at some point going forward.

ie: you're suggesting users needing to use DWARFv4 line tables would access that functionality with a flag passed to the assembler (as they do today if they want to opt in to DWARFv5)?

Right, yes, that's what I'm suggesting.

I think part of the problem is that people aren't used to (ie: build systems and other infrastructure isn't designed for) having to pass a dwarf version to their assembler - the line table format hasn't changed since DWARFv2. So if you have run -gN -S, you can pass the resulting file to the compiler/assembler without any flags. So even if we switch the default - then people who aren't ready to switch to DWARFv4 are going to be in a possibly difficult situation of trying to teach their build system how to pass the appropriate flags to the assembler, etc.

I think maybe the way to think about it is that there wouldn't be a default DWARF version - it could always be determined by the presence/absence of a file 0.

I'm concerned about relying on what is essentially a minor detail of the specification, for a couple of reasons:

  1. What if the .file 0 directive isn't present for future DWARF versions (e.g. DWARFv6), for whatever reason? You might have some code that is generated expecting DWARFv6, yet the assembler will end up generating (or at least trying to generate) DWARFv4 output. It seems like relying on a detail like this is fragile and not particularly future-proof.
  2. To most users, it won't be clear that .file 0 means DWARFv5 and no .file 0 means DWARFv4. This could lead to potential confusion when the DWARF doesn't contain the expected information.

Maybe a silly suggestion (and one that would need more discussion on the mailing lists), but why not just change the default version to 5 more widely? How often do people deliberately deviate from the default DWARF version in practice? To me, the proposed change feels like a bit of a hack for what is likely fairly unusual usage, and is only temporary, since likely most toolchains will want to change to DWARF v5 at some point going forward.

ie: you're suggesting users needing to use DWARFv4 line tables would access that functionality with a flag passed to the assembler (as they do today if they want to opt in to DWARFv5)?

Right, yes, that's what I'm suggesting.

I think part of the problem is that people aren't used to (ie: build systems and other infrastructure isn't designed for) having to pass a dwarf version to their assembler - the line table format hasn't changed since DWARFv2. So if you have run -gN -S, you can pass the resulting file to the compiler/assembler without any flags. So even if we switch the default - then people who aren't ready to switch to DWARFv4 are going to be in a possibly difficult situation of trying to teach their build system how to pass the appropriate flags to the assembler, etc.

I think maybe the way to think about it is that there wouldn't be a default DWARF version - it could always be determined by the presence/absence of a file 0.

I'm concerned about relying on what is essentially a minor detail of the specification, for a couple of reasons:

  1. What if the .file 0 directive isn't present for future DWARF versions (e.g. DWARFv6), for whatever reason? You might have some code that is generated expecting DWARFv6, yet the assembler will end up generating (or at least trying to generate) DWARFv4 output. It seems like relying on a detail like this is fragile and not particularly future-proof.

Perhaps add an assembly directive when that that time comes?

  1. To most users, it won't be clear that .file 0 means DWARFv5 and no .file 0 means DWARFv4. This could lead to potential confusion when the DWARF doesn't contain the expected information.

Looks like they have made the change to GNU as: https://sourceware.org/bugzilla/show_bug.cgi?id=27195

Maybe a silly suggestion (and one that would need more discussion on the mailing lists), but why not just change the default version to 5 more widely? How often do people deliberately deviate from the default DWARF version in practice? To me, the proposed change feels like a bit of a hack for what is likely fairly unusual usage, and is only temporary, since likely most toolchains will want to change to DWARF v5 at some point going forward.

ie: you're suggesting users needing to use DWARFv4 line tables would access that functionality with a flag passed to the assembler (as they do today if they want to opt in to DWARFv5)?

Right, yes, that's what I'm suggesting.

I think part of the problem is that people aren't used to (ie: build systems and other infrastructure isn't designed for) having to pass a dwarf version to their assembler - the line table format hasn't changed since DWARFv2. So if you have run -gN -S, you can pass the resulting file to the compiler/assembler without any flags. So even if we switch the default - then people who aren't ready to switch to DWARFv4 are going to be in a possibly difficult situation of trying to teach their build system how to pass the appropriate flags to the assembler, etc.

I think maybe the way to think about it is that there wouldn't be a default DWARF version - it could always be determined by the presence/absence of a file 0.

I'm concerned about relying on what is essentially a minor detail of the specification, for a couple of reasons:

  1. What if the .file 0 directive isn't present for future DWARF versions (e.g. DWARFv6), for whatever reason? You might have some code that is generated expecting DWARFv6, yet the assembler will end up generating (or at least trying to generate) DWARFv4 output. It seems like relying on a detail like this is fragile and not particularly future-proof.

Perhaps add an assembly directive when that that time comes?

What's stopping us adding an assembly directive now? The sooner we add it, the less chance there'll be old assembly kicking around that causes problems.

  1. To most users, it won't be clear that .file 0 means DWARFv5 and no .file 0 means DWARFv4. This could lead to potential confusion when the DWARF doesn't contain the expected information.

Looks like they have made the change to GNU as: https://sourceware.org/bugzilla/show_bug.cgi?id=27195

As noted here, I think that upgrade is a mistake personally, and that an assembly directive would be a clearer and cleaner solution. However, I suppose there is the question of what to do about assembly files without the proposed directive, and that might be to do the upgrade in that case, I guess, although I'm not sure whether that's a real case that needs supporting.

Catching up on this, sorry...

I believe the file number in .loc has to be defined by a preceding .file directive; at least, a .s file with a bare .loc 1 1 1 gets an error about "unassigned file number 1" from as. So, .file 0 is the only special case with regards to picking a DWARF version.

I'm not keen on defining an assembler directive that GNU as does not also support.

I don't mind upgrading MC's opinion of the *defaulted* DWARF version, based on the existence of a .file 0 directive. I'd be less happy about llvm-mc -dwarf-version 4 foo.s emitting DWARF v5 line tables, because the user asked for something very specifically, in a developer tool.

Clang does not provide a user-level assembler tool; assembler source is passed through Clang itself.
clang -c -g foo.s implies that foo.s has no embedded DWARF directives, and the user wants the DWARF to describe the assembler source itself. If we do see DWARF directives, then we abandon the idea of describing the assembler source, and take the embedded DWARF directives as determining what to do. I think I would not be opposed to emitting DWARF v5 in this case, if we saw a .file 0.

The counter-argument to that last bit is that we do obey the specified version if there is one; i.e., clang -c -gdwarf-3 foo.s will emit DWARF v3 line tables, not v4. This suggests that if we see a .file 0 we should complain; and then why shouldn't we complain in the simple clang -g case?

But if the .s file has a .file 0 directive (or an md5 keyword, or anything else new in v5) that implies the .s file was generated with v5 in mind, and so producing an object file with v5 line tables would not be a practical problem.

A few thoughts:

  1. If gnu-as already has accepted such a patch, we probably want to do the same just for compatibility
  2. it doesn't seem like adding a directive sooner (especially if it risks diverging from gnu-as if they add one in the future - they don't tend to be terribly interested in compatibility with clang from my experience) significantly reduces work here, especially if we're doing (1) - add the directive when it becomes necessary if file 0 becomes invalid in DWARFv6 or something (though I don't expect it to any time soon) or more likely if a new line table format arises with significant benefits/incompatibilities with the v5 line table format
  3. I'd be pretty happy with clang -gdwarfN and llvm-mc -dwarf-version doing the same thing & ignoring the specified format if the input contains dwarf of a different version - not too fussed in any direction for llvm-mc though.

A few thoughts:

  1. If gnu-as already has accepted such a patch, we probably want to do the same just for compatibility

Agree.

  1. it doesn't seem like adding a directive sooner (especially if it risks diverging from gnu-as if they add one in the future - they don't tend to be terribly interested in compatibility with clang from my experience) significantly reduces work here, especially if we're doing (1) - add the directive when it becomes necessary if file 0 becomes invalid in DWARFv6 or something (though I don't expect it to any time soon) or more likely if a new line table format arises with significant benefits/incompatibilities with the v5 line table format

Agree that a directive may not be sooner. If the line table format is going to be changed, the directive can probably be deferred to that time.

  1. I'd be pretty happy with clang -gdwarfN and llvm-mc -dwarf-version doing the same thing & ignoring the specified format if the input contains dwarf of a different version - not too fussed in any direction for llvm-mc though.

I just checked latest binutils after https://sourceware.org/bugzilla/show_bug.cgi?id=27195

gas/as-new a.s -gdwarf-3 -o a.o happily upgrades to DWARF v5 upon .file 0 (https://sourceware.org/pipermail/binutils/2021-January/114981.html was not adopted). Therefore this patch will match the behavior entirely, although I slightly dislike that explicit -gdwarf-N is not respected (as I said in the description).

Okay, I'm persuaded by the arguments. I don't really mind whether the tool will ignore an explicitly specified version or not, as long as a warning is emitted if it does ignore it. I think we want to avoid the potential confusion of "I asked for DWARFv3, but am getting DWARFv5 output", and a warning would at least help with that (assuming of course we don't just refuse to handle this case, which I think is also an acceptable, and possibly preferable solution).

Okay, I'm persuaded by the arguments. I don't really mind whether the tool will ignore an explicitly specified version or not, as long as a warning is emitted if it does ignore it. I think we want to avoid the potential confusion of "I asked for DWARFv3, but am getting DWARFv5 output", and a warning would at least help with that (assuming of course we don't just refuse to handle this case, which I think is also an acceptable, and possibly preferable solution).

GNU as hasn't implemented the diagnostic yet.

Implementing the warning in MC needs a new option (clang -c a.c passes -dwarf-version to cc1as even in the absence of -g).

Okay, I'm persuaded by the arguments. I don't really mind whether the tool will ignore an explicitly specified version or not, as long as a warning is emitted if it does ignore it. I think we want to avoid the potential confusion of "I asked for DWARFv3, but am getting DWARFv5 output", and a warning would at least help with that (assuming of course we don't just refuse to handle this case, which I think is also an acceptable, and possibly preferable solution).

GNU as hasn't implemented the diagnostic yet.

Implementing the warning in MC needs a new option (clang -c a.c passes -dwarf-version to cc1as even in the absence of -g).

Opinions? :)

I'm a little lost as to what you're referring to requiring a new command line option. If I understand it correctly, the issue is when an assembly file was produced with a specific version which doesn't match what is used when it gets assembled.

Let me try and reframe so to help clear things up. We have the following cases:

  1. No option is passed to the assembler - in this case, the assembler should be able to infer the DWARF version from the presence (or lack thereof) of .file 0, no diagnostic needed.
  2. The DWARF version is passed to the assembler via an option. In this case, in my opinion should warn if it encounters .file 0 but the requested version doesn't support .file 0. Potentially, the inverse should also warn too (i.e. .file 0 is not present, but other .file directives are, and version 5 is requested), but I am not familiar enough with the related assembly to know whether that is supported or not.

Does clang need to pass dwarf-version to cc1as at all when the version is not explicitly specified/debug data is not specifically requested? If it could avoid specifying that option, we'd be able to hit case 1) for most cases, I think, whilst still getting the warning for case 2 when there's a mismatch.

I'm a little lost as to what you're referring to requiring a new command line option. If I understand it correctly, the issue is when an assembly file was produced with a specific version which doesn't match what is used when it gets assembled.

Let me try and reframe so to help clear things up. We have the following cases:

  1. No option is passed to the assembler - in this case, the assembler should be able to infer the DWARF version from the presence (or lack thereof) of .file 0, no diagnostic needed.

Yes, GNU as behavior.

  1. The DWARF version is passed to the assembler via an option. In this case, in my opinion should warn if it encounters .file 0 but the requested version doesn't support .file 0. Potentially, the inverse should also warn too (i.e. .file 0 is not present, but other .file directives are, and version 5 is requested), but I am not familiar enough with the related assembly to know whether that is supported or not.

I agree that a warning would be nice.

Does clang need to pass dwarf-version to cc1as at all when the version is not explicitly specified/debug data is not specifically requested? If it could avoid specifying that option, we'd be able to hit case 1) for most cases, I think, whilst still getting the warning for case 2 when there's a mismatch.

The default DWARF version is decided by clang driver. Different toolchains have different defaults. The version option does not require -g.

clang -target x86_64-linux -c a.s '-###' # "-dwarf-version=4"
clang -target x86_64-freebsd12 -c a.s '-###' # "-dwarf-version=2"

If we want a diagnostic, we will need additional information whether -dwarf-version is implicitly or explicitly specified. GNU as currently does not have a diagnostic.
I have put reason why I think the diagnostic is not very useful in my initial summary.

The default DWARF version is decided by clang driver. Different toolchains have different defaults. The version option does not require -g.

clang -target x86_64-linux -c a.s '-###' # "-dwarf-version=4"
clang -target x86_64-freebsd12 -c a.s '-###' # "-dwarf-version=2"

If we want a diagnostic, we will need additional information whether -dwarf-version is implicitly or explicitly specified. GNU as currently does not have a diagnostic.
I have put reason why I think the diagnostic is not very useful in my initial summary.

Got it now, thanks. I don't have a strong feeling about whether a new option to enable this diagnostic is a good idea or not. If it existed, I'd expect to be something like --warn-for-mismatching-dwarf-version or something to that effect, and is specified automatically if the user explicitly specifies the DWARF version themselvse. I don't think whether GNU as produces a diagnostic or not is really relevant. I think I'd like someone else with more frontend experience to chime in at this point.

The default DWARF version is decided by clang driver. Different toolchains have different defaults. The version option does not require -g.

clang -target x86_64-linux -c a.s '-###' # "-dwarf-version=4"
clang -target x86_64-freebsd12 -c a.s '-###' # "-dwarf-version=2"

If we want a diagnostic, we will need additional information whether -dwarf-version is implicitly or explicitly specified. GNU as currently does not have a diagnostic.
I have put reason why I think the diagnostic is not very useful in my initial summary.

Got it now, thanks. I don't have a strong feeling about whether a new option to enable this diagnostic is a good idea or not. If it existed, I'd expect to be something like --warn-for-mismatching-dwarf-version or something to that effect, and is specified automatically if the user explicitly specifies the DWARF version themselvse. I don't think whether GNU as produces a diagnostic or not is really relevant. I think I'd like someone else with more frontend experience to chime in at this point.

The issue isn't so much needing a flag to enable the warning, but needing a flag to pass down/differentiate explicit versus implicitly specified version number (currently we don't have that differentiation in the backend - so MCStreamer couldn't figure out whether to warn or not). But, yes, would also then want a flag for it.

I don't think a warning here is necessary - if someone's got a -gdwarf-4 flag specified in their build system but some DWARFv5 in an assembly file, there's nothing much to do but emit the DWARFv5 they have & it's probably fine. Indeed, passing any -g flag when you're assembling a file that already has DWARF in it is not necessary - arguably the flags are irrelevant/ignored/unrelated to the task at hand. (I guess not entirely true because we probably still pick the line table version from within the ones that would be valid given the line directives, using the command line flag)

I wouldn't stand in the way of someone adding a warning - with all the requisite support for plumbing through the needed flags (to support the necessary differentiation between implicit and explicit version and the ability to enable/disable it), but I wouldn't hold up this patch on that/suggest that that work needs to be done in conjunction with this.

...
I wouldn't stand in the way of someone adding a warning - with all the requisite support for plumbing through the needed flags (to support the necessary differentiation between implicit and explicit version and the ability to enable/disable it), but I wouldn't hold up this patch on that/suggest that that work needs to be done in conjunction with this.

(That is also my current feeling. The additional plumbing & the perceived low value made me think I will add the diagnostic)
Ping @jhenderson 😊

I don't have any particularly strong feelings on the matter, and don't have enough knowledge about the plumbing, so no objections from me for this to proceed further as others think is best.

dblaikie accepted this revision.Tue, Feb 2, 9:18 AM

Looks good, thanks!

This revision is now accepted and ready to land.Tue, Feb 2, 9:18 AM
This revision was automatically updated to reflect the committed changes.