Page MenuHomePhabricator

[DWARF] Allow toolchain to adjust specified DWARF version.
ClosedPublic

Authored by tra on Dec 3 2020, 3:57 PM.

Details

Summary

This is needed for CUDA compilation where NVPTX back-end only supports DWARF2,
but host compilation should be allowed to use newer DWARF versions.

Diff Detail

Event Timeline

tra created this revision.Dec 3 2020, 3:57 PM
tra requested review of this revision.Dec 3 2020, 3:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 3 2020, 3:57 PM
dblaikie added inline comments.Dec 3 2020, 4:19 PM
clang/include/clang/Driver/ToolChain.h
496

Given these semantics (pass/return the version) maybe "AdjustDwarfVersion" or the like might be more suitable (are there other similar functions in this API That can/do this sort of pass/return API?).

Alternatively, a narrower/simpler API could be "GetMaxDwarfVersion", say, which just returns int_max by default and 2 for NVPTX?

clang/lib/Driver/ToolChains/Clang.cpp
3848

I worry a bit about having both DWARFVersion and AdjustedDwarfVersion in this scope (I'd worry about some codepaths using one when they needed to use the other, etc) - if at all possible, would be good to just override DWARFVersion with the adjusted value. And probably could/should be up in the "EmitDwarf" block above that's handling default dwarf versions and overrides of it?

tra updated this revision to Diff 309404.Dec 3 2020, 4:46 PM
tra edited the summary of this revision. (Show Details)

Updated according to Devid's feedback.

tra added inline comments.Dec 3 2020, 4:48 PM
clang/lib/Driver/ToolChains/Clang.cpp
3848

Ive updated the implementation to make it less ambiguous. PTAL.

dblaikie added inline comments.Dec 3 2020, 4:52 PM
clang/lib/Driver/ToolChains/Clang.cpp
3948

I think, ideally, the DWARFVersion calculation would happen up if (EmitDwarf) { block where all default and explicit dwarf version calculations are done.

I guess it's not done that way because of the gembed-source error path?

tra added inline comments.Dec 3 2020, 8:01 PM
clang/lib/Driver/ToolChains/Clang.cpp
3948

That's part of it. -gembed-source does need to know both the specified DWARF version and the clamped one. I could move the calculation of the effective DWARF version back to where it was in the first version of the patch, and we'll need both the specified and the clamped values.
That would bring back the need to explain which of the two DWARF versions to use, where, and why. In the current revision the impact on DWARF version clamping is localized to where it makes a difference. Neither is perfect, but the current version is a bit easier to explain, I think.

As for moving it under if( EmitDwarf), the problem is that the DWARF version also affects cases when EmitDwarf is false, so the clamping needs to be done regardless. E.g. the -gmlt option that was used in our build break that uncovered the issue. We do not emit DWARF per se, but the DefaultDWARFVersion does affect the generation of the lineinfo debugging and we do need to clamp it.

MaskRay added a subscriber: MaskRay.Dec 3 2020, 8:05 PM
MaskRay added inline comments.
clang/test/Driver/cuda-unsupported-debug-options.cu
2

A NOT pattern may easily become stale and do not actually check anything. Just turn to a positive pattern?

tra added inline comments.Dec 3 2020, 8:12 PM
clang/test/Driver/cuda-unsupported-debug-options.cu
2

In this case the issue is with the CHECK-NOT line above. I'll have to replicate it around the positive match of -dwarf-version which would raise more questions.

I wish filecheck would allow to mark a region and then run multiple matches on it, instead of re-anchoring on each match.

dblaikie added inline comments.Dec 3 2020, 8:25 PM
clang/lib/Driver/ToolChains/Clang.cpp
3948

Is the warning necessary? Could we let -gembed-source be an error if you're targeting NVPTX? (I realize that goes a bit against the idea of this patch, which is to keep DWARFv2 even when you've got a bunch of DWARFv5 flags that you presumably want for the rest of your builds)

Or, I guess another option, might be to make -gembed-source a warning for non-v5 DWARF in general, instead of an error, then the implementation could be generic.

What I'd really like is for the DWARFVersion to be computed as compactly as possible without the concept of multiple versions leaking out as much as possible.

As for moving it under if( EmitDwarf), the problem is that the DWARF version also affects cases when EmitDwarf is false, so the clamping needs to be done regardless. E.g. the -gmlt option that was used in our build break that uncovered the issue.

That doesn't sound right to me, -gmlt qualifies as "emitting DWARF" and at least a quick/simple debug through the driver for "clang x.cpp -gmlt" does seem to lead to "EmitDwarf" being true and the block at line 3841 being reached. EmitDwarf is set to true in that case at 3834 because DebugInfoKind is not NoDebugInfo (it's DebugLineTablesOnly)

tra updated this revision to Diff 309585.Dec 4 2020, 11:07 AM

Simplified dwarf version clamping.

tra added inline comments.Dec 4 2020, 11:08 AM
clang/lib/Driver/ToolChains/Clang.cpp
3948

Is the warning necessary? Could we let -gembed-source be an error if you're targeting NVPTX? (I realize that goes a bit against the idea of this patch, which is to keep DWARFv2 even when you've got a bunch of DWARFv5 flags that you presumably want for the rest of your builds)

I think the warning is the sensible thing to do here. Making it an error would effectively make it impossible to use it on the host side of the compilation where it's perfectly fine and where it's most useful.
At the same time, we already issue a warning for some debugging options if a back-end does not support them, so a warning for -gembed-source follows the established pattern, even if it's handled as a special case.

Or, I guess another option, might be to make -gembed-source a warning for non-v5 DWARF in general, instead of an error, then the implementation could be generic.

That would work. Then we'll only need the capped dwarf version and that could be moved upward. I've updated the patch. PTAL.

that doesn't sound right to me, -gmlt qualifies as "emitting DWARF"

You are correct. Even when we only emit debug info via .loc, EmitDwarf is set.

dblaikie added inline comments.
clang/test/Driver/cuda-unsupported-debug-options.cu
2

Sounds like you're looking for CHECK-DAG, maybe? ( https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-dag-directive )

clang/test/Driver/debug-options.c
364–366 ↗(On Diff #309585)

This is a bit of a loss in fidelity - might need a new diagnostic message (or go hunting around for a more general purpose one than this one at least) to say '-gembed-source' is ignored when not using DWARFv5. (in the nvptx case it's "not supported on target", but in the existing cases covered by this test it's "not supported because the user requested DWARFv2", for instance)

@JDevlieghere & @scott.linder for thoughts on this.

scott.linder added inline comments.Dec 4 2020, 1:12 PM
clang/test/Driver/debug-options.c
364–366 ↗(On Diff #309585)

I agree that I'd prefer we detect whether the target-specific clamped version is to blame (and use the proposed warning message) or the original DWARF version is to blame (and use the old message).

If I were compiling for x86 and gave e.g. -gdwarf-4 -gembed-source and the error said "not supported by target" I'd probably get the wrong idea.

It would also be nice to retain the error semantics in the case where the user is explicitly requesting incompatible options.

tra added inline comments.Dec 4 2020, 1:50 PM
clang/test/Driver/cuda-unsupported-debug-options.cu
2

I don't think CHECK-DAG can be mixed with CHECK-NOT. At least, not the way I need them here. From FileCheck docs:

CHECK-NOT: directives could be mixed with CHECK-DAG: directives to exclude strings between the surrounding CHECK-DAG: directives.

So, in order to use it here I'll need something like this:

// CHECK: -triple // GPU-side compilation
// CHECK-NOT: {{all unsupported options}}
// CHECK: -dwarf-version=2  // Could be CHECK-DAG, too, it does not matter in this case.
// We have to repeat the NOT check here because the positive check above creates another subset of input to check for -NOT.
// CHECK-NOT: {{all unsupported options}} 
// CHECK: - triple // Host-side compilation

Ideally I want something like this:

// CHECK-WITHOUT-ADVANCE: -dwarf-version=2
// CHECK-NOT: {{unsupported options}}

If you prefer positive check with replicated -NOT, I'm fine with that.

clang/test/Driver/debug-options.c
364–366 ↗(On Diff #309585)

This sounds pretty close to what the older iterations of this patch did: https://reviews.llvm.org/D92617?id=309404, except that it preserved the current behavior which makes it an error to use -gembed-source with an explicitly specified DWARF version below 5. Same options with a lower clamped version produces a warning. I.e. If user specified a nominally valid combination of -gdwarf-5 -gembed-source but the target like NVPTX clamped it down to DWARF2, there will only be a warning.

I would appreciate if you (as in debug info stakeholders) could clarify couple of moments for me:

  • should -gdwarf-4 -gembed-source be an error or warning? It's currently an error.
  • -gdwarf-5 -gembed-source with the dwarf version clamped below 5 should produce a warning. not supported for target appears to be correct, but does not explain why. Do we want to amend/change it to say because target only supports DWARF 2 or target does not support DWARF 5? Or is not supported by target sufficient as is?
scott.linder added inline comments.Dec 4 2020, 2:34 PM
clang/test/Driver/debug-options.c
364–366 ↗(On Diff #309585)

This sounds pretty close to what the older iterations of this patch did: https://reviews.llvm.org/D92617?id=309404, except that it preserved the current behavior which makes it an error to use -gembed-source with an explicitly specified DWARF version below 5. Same options with a lower clamped version produces a warning. I.e. If user specified a nominally valid combination of -gdwarf-5 -gembed-source but the target like NVPTX clamped it down to DWARF2, there will only be a warning.

I would appreciate if you (as in debug info stakeholders) could clarify couple of moments for me:

  • should -gdwarf-4 -gembed-source be an error or warning? It's currently an error.

I think it should remain an error when an incompatible DWARF version is explicitly specified by the user. They said they wanted two things which are mutually exclusive, and we have no way to know which one they meant (or if they just aren't aware they are incompatible) so we should stop and prompt them to fix it.

  • -gdwarf-5 -gembed-source with the dwarf version clamped below 5 should produce a warning. not supported for target appears to be correct, but does not explain why. Do we want to amend/change it to say because target only supports DWARF 2 or target does not support DWARF 5? Or is not supported by target sufficient as is?

I think giving more context is a good thing in this situation; I don't know that I have a strong opinion on the wording, but something indicating the warning is due to a target restriction which caps the DWARF version seems helpful enough to warrant being more verbose. I'd vote for your because target only supports DWARF <N>

dblaikie added inline comments.Dec 4 2020, 6:32 PM
clang/test/Driver/debug-options.c
364–366 ↗(On Diff #309585)

OK, sounds like we're going mostly back towards the originally proposed code.

Fair enough - if that's the case I'd make one suggestion to tweak the original code:

Rather than keeping DWARFVersion unclamped and having AdjustedDWARFVersion as well - It might work better to have DWARFVersion updated correctly (in the "if (EmitDwarf)" block) and save "UnadjustedDWARFVersion" (open to other naming suggestions) so that vaule can be consulted later to determine the appropriate error or warning to be emitted. But this way "DWARFVersion" does carry the to-be-emitted DWARF version which is more likely what the next developer to touch this code will be thinking about/wanting when they reach for that variable.

tra updated this revision to Diff 310019.Dec 7 2020, 2:09 PM

Updated to address the comments. PTAL.

dblaikie added inline comments.Dec 7 2020, 2:49 PM
clang/include/clang/Basic/DiagnosticDriverKinds.td
296

Probably worth testing the rest of this error message to check the versions and target names all show up as intended (looks like the test currently glosses over the differences between this warning and the other error?) - and also testing that it's emitted as a warning, whereas the other/pre-existing diagnostic was emitted as an error (oh, I guess perhaps it already is, just in another file - could you check that it is tested narrowly/ensured it's an error elsewhere?)

clang/lib/Driver/ToolChains/Clang.cpp
3920–3921

Use "else if {" on one line here, perhaps?

tra updated this revision to Diff 310061.Dec 7 2020, 5:00 PM

Addressed comments.

tra updated this revision to Diff 310062.Dec 7 2020, 5:11 PM
tra marked an inline comment as done.

Adjusted openmp test for the changed -gembed-source warning.

dblaikie added inline comments.Dec 7 2020, 5:41 PM
clang/test/Driver/cuda-unsupported-debug-options.cu
1

What's the purpose of the {{.*}} in this line?

clang/test/Driver/openmp-unsupported-debug-options.c
0

also curious about the {{.*}} here too - but I guess it's the same answer as the other spot?

Harbormaster completed remote builds in B81379: Diff 310062.
tra added inline comments.Dec 8 2020, 4:38 PM
clang/include/clang/Basic/DiagnosticDriverKinds.td
296

Probably worth testing the rest of this error message to check the versions and target names all show up as intended

Done.

(looks like the test currently glosses over the differences between this warning and the other error?) - and also testing that it's emitted as a warning, whereas the other/pre-existing diagnostic was emitted as an error (oh, I guess perhaps it already is, just in another file - could you check that it is tested narrowly/ensured it's an error elsewhere?)

The error case is already tested in https://github.com/llvm-mirror/clang/blob/master/test/Driver/debug-options.c#L355

clang/test/Driver/cuda-unsupported-debug-options.cu
1

The wildcard eats the differences between two warnings -- a generic one:

  • warning: debug information option '-glldb' is not supported for target 'nvptx64-nvidia-cuda'

and the more specific one for -gembed-source :

  • warning: debug information option '-gembed-source' is not supported. It needs DWARF-5 but target 'nvptx64-nvidia-cuda' only provides DWARF-2.
clang/test/Driver/openmp-unsupported-debug-options.c
0

Yes, same here.

dblaikie added inline comments.Dec 8 2020, 9:20 PM
clang/test/Driver/cuda-unsupported-debug-options.cu
1

Is that necessary? Or is the -gembed-source+CHECK test redundant with/less precise than the -gembed-source+DWARF-CLAMP test? (ie: would it be effective (not a loss in test coverage) to remove the gembed-source+CHECK test and only have the gembed-source+DWARF-CLAMP test, and keep the CHECK line precise as it was before for all the CHECK tests)

Effectively: Rather than adding a new RUN line, and generalizing the CHECK over the new/old message and checking the new message separately - instead, change the existing gembed-source test to use DWARF-CLAMP instead of CHECK, and leave everything the same?

tra updated this revision to Diff 310622.Dec 9 2020, 12:53 PM

Reorganized tests for unsupported debug options & dwarf version clamping.

tra added inline comments.Dec 9 2020, 12:57 PM
clang/test/Driver/cuda-unsupported-debug-options.cu
1

CHECK lines test that an option was ignored for the right target (nvptx, not x86). So, both warning variants should participate.

DWARF-CLAMP verify that the correct DWARF version was used.

-gembed-source shoud participate in both tests. The options here is to either include -gembed-source in the CHECK tests and make it accept both pattern variants, or replicate the target of the warning check within DWARF-CLAMP.

I would prefer to keep these two kinds of tests separate.

I've just updated the patch and moved DWARF clamping tests into a separate file and consolidated nearly-identical cuda/omp unsupported option warnings tests into the same file.

dblaikie accepted this revision.Dec 9 2020, 1:10 PM

Fair enough - thanks for your patience/explanations!

This revision is now accepted and ready to land.Dec 9 2020, 1:10 PM
This revision was automatically updated to reflect the committed changes.