tra added inline comments.

================
Comment at: clang/test/Driver/cuda-unsupported-debug-options.cu:22
+// Make sure we do not see any dwarf version other than 2, regardless of 
what's used on the host side.
+// CHECK-NOT: {{-dwarf-version=[^2]}}
 // CHECK: "-triple" "x86_64
----------------
dblaikie wrote:
> tra wrote:
> > MaskRay wrote:
> > > A NOT pattern may easily become stale and do not actually check anything. 
> > > Just turn to a positive pattern?
> > 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. 
> Sounds like you're looking for CHECK-DAG, maybe? ( 
> https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-dag-directive )
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.


================
Comment at: clang/test/Driver/debug-options.c:364-366
+// GEMBED_2:  warning: debug information option '-gembed-source' is not 
supported for target
 // NOGEMBED_5-NOT:  "-gembed-source"
+// NOGEMBED_2-NOT:  warning: debug information option '-gembed-source' is not 
supported for target
----------------
scott.linder wrote:
> dblaikie wrote:
> > 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.
> 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.
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?




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92617/new/

https://reviews.llvm.org/D92617

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to