driver: add support for a more generic diagnostic handler #4302

ljmf00 · 2023-01-22T02:32:07Z

Signed-off-by: Luís Ferreira contact@lsferreira.net

I can't add you as a reviewer @JohanEngelen since I don't have permissions on LDC org. This is something I need cherry-picked in weka to diagnose traces with possibly bigger stack usage than permitted.

The default seem to warn on stdout rather than stderr and also doesn't respect -w and -wi command line options and therefore it seem to not working on our build system diagnostics.

This also further improves the existing logic on diagnostic handler as it was wrongly checking the DiagnosticInfoSrcMgr severity and not incrementing the error counter.

I'm going to add tests for this tomorrow.

ljmf00 · 2023-01-22T02:34:36Z

Perhaps, we can get rid of default llvm diagnostic printer and use DMD error/warning functions.

thewilsonator · 2023-01-22T07:56:36Z

Do you have a test case for this?

ljmf00 · 2023-01-22T12:14:39Z

tests/codegen/attr_warn_stack_size.d

+
+import ldc.attributes;
+
+// CHECK: {{.*}}Warning: stack frame size (8) exceeds limit (1) in '_D20attr_warn_stack_size6foobarFiZf'


I'm defering to LLVM demangler. I'm working on upstreaming the demangler, but for now it like this, should I abstract this from the test too?

you can also use {{.*}}foobar to not depend on yes/no demangling. If demangling works, I'd make it part of the test, if it doesn't then remove demangling yes/no assumption from the test.

ljmf00 · 2023-01-22T12:15:09Z

Do you have a test case for this?

From my PR description :)

I'm going to add tests for this tomorrow.

JohanEngelen · 2023-01-22T12:40:49Z

driver/codegenerator.cpp

+#if LDC_LLVM_VER > 1400
+        llvm::StringRef fname;
+        unsigned line, column;
+        DISS.getLocation(fname, line, column);


is getting the location generalizable for all cases? (i.e. call it before switch using DI)

I think I can change this to use the IR state loc, when its not available through the diagnostic info object.

JohanEngelen · 2023-01-22T12:43:06Z

driver/codegenerator.cpp

+#else
+        const auto loc = Loc(nullptr, 0, 0);
+#endif
+        switch (DISS.getSeverity())


this also seems generalizable (i.e. not individual switches for severity for each kind of diag info type?)

driver/codegenerator.cpp

JohanEngelen · 2023-01-22T12:46:56Z

driver/codegenerator.cpp

+            error(loc, "stack frame size (%ld) exceeds limit (%ld) in '%s'",
+              DISS.getStackSize(),
+              DISS.getStackLimit(),
+              llvm::demangle(DISS.getFunction().getName().str()).c_str()


do we use the LLVM demangler anywhere else in the codebase? Not better to use D's own demangler?

(because then it is always up-to-date, when building LDC with itself)

(because then it is always up-to-date, when building LDC with itself)

I don't think so. Makes sense to use the official one due to that, but the tradeoff is that is very slow. core.demangle use exceptions for control flow, which is an horrible implementation. I have a project to update the official demangler to a decent and fast parser, but I endedup getting out of time for these things :/

The compiler is erroring, don't worry about performance, it's fine if it takes an extra second even. ;)

JohanEngelen · 2023-01-22T12:49:39Z

Nice. I wonder if there are other LLVM attributes that can generate errors/warnings, such as @target. If so, please make a TODO issue :)

Edit: even if we defer for other diagnostic kinds to the default LLVM's diagnostic function, we can still ++ global.errors or warnings, such that -w is respected.

tests/codegen/attr_warn_stack_size.d

ljmf00 · 2023-01-22T13:04:12Z

Edit: even if we defer for other diagnostic kinds to the default LLVM's diagnostic function, we can still ++ global.errors or warnings, such that -w is respected.

Yeah, makes sense to me, but keep in mind that apparently, warning severities does write to stdout.

ljmf00 · 2023-01-22T13:35:15Z

I can't really understand what I'm doing wrong in the test. Maybe its trivial but I'm not getting there.

Signed-off-by: Luís Ferreira <contact@lsferreira.net>

kinke · 2023-01-22T14:18:10Z

Btw, wrt. ++global.errors etc. - according to #4293, there's a missing check for such LLVM errors.

JohanEngelen · 2023-01-22T14:51:02Z

I can't really understand what I'm doing wrong in the test. Maybe its trivial but I'm not getting there.

To run lit-based tests locally, go into your build folder, go to tests folder, then run ./runlit.py -v . or ./runlit.py -v codegen/attr_warn_stack_size.d.

JohanEngelen · 2023-01-22T14:52:17Z

tests/codegen/attr_warn_stack_size.d

+
+// REQUIRES: atleast_llvm1400
+
+// RUN: %ldc -wi -c %s 2>&1 | FileCheck %s


This should be a not if you want it to error out, I think you had that correct in the first commit. But you need a fix for kinke's comment: #4293

ljmf00 · 2023-01-23T09:57:00Z

Btw, wrt. ++global.errors etc. - according to #4293, there's a missing check for such LLVM errors.

Yes, it fixes it! I'll also add a test case for it.

…m errors Based on Luís' ldc-developers#4302.

…m errors Based on Luís' ldc-developers#4302; fixes ldc-developers#4293.

…op compilation Based on Luís' ldc-developers#4302; fixes ldc-developers#4293.

…op compilation (#4331) Based on Luís' #4302; fixes #4293.

ljmf00 force-pushed the use-warning-stack-size-diag branch from ef1460b to 65c7215 Compare January 22, 2023 12:13

ljmf00 commented Jan 22, 2023

View reviewed changes

JohanEngelen reviewed Jan 22, 2023

View reviewed changes

driver/codegenerator.cpp Outdated Show resolved Hide resolved

JohanEngelen reviewed Jan 22, 2023

View reviewed changes

ljmf00 force-pushed the use-warning-stack-size-diag branch 2 times, most recently from a539f13 to 2e943f8 Compare January 22, 2023 12:51

JohanEngelen reviewed Jan 22, 2023

View reviewed changes

tests/codegen/attr_warn_stack_size.d Outdated Show resolved Hide resolved

ljmf00 force-pushed the use-warning-stack-size-diag branch from 2e943f8 to 2fe9396 Compare January 22, 2023 13:00

driver: add support for a more generic diagnostic handler

626823c

Signed-off-by: Luís Ferreira <contact@lsferreira.net>

ljmf00 force-pushed the use-warning-stack-size-diag branch from 2fe9396 to 626823c Compare January 22, 2023 13:55

JohanEngelen reviewed Jan 22, 2023

View reviewed changes

kinke added a commit to kinke/ldc that referenced this pull request Feb 26, 2023

Fix regression with LLVM 13+: Return non-zero exit code for inline as…

1491dc2

…m errors Based on Luís' ldc-developers#4302.

kinke mentioned this pull request Feb 26, 2023

Fix regression with LLVM 13+: some errors in inline assembly don't stop compilation #4331

Merged

kinke added a commit to kinke/ldc that referenced this pull request Feb 26, 2023

Fix regression with LLVM 13+: Return non-zero exit code for inline as…

671cf70

…m errors Based on Luís' ldc-developers#4302; fixes ldc-developers#4293.

kinke added a commit to kinke/ldc that referenced this pull request Feb 26, 2023

Fix regression with LLVM 13+: some errors in inline assembly don't st…

c67c6e1

…op compilation Based on Luís' ldc-developers#4302; fixes ldc-developers#4293.

kinke added a commit to kinke/ldc that referenced this pull request Feb 26, 2023

Fix regression with LLVM 13+: some errors in inline assembly don't st…

bad2ca9

…op compilation Based on Luís' ldc-developers#4302; fixes ldc-developers#4293.

kinke added a commit that referenced this pull request Feb 26, 2023

Fix regression with LLVM 13+: some errors in inline assembly don't st…

7d3f5ba

…op compilation (#4331) Based on Luís' #4302; fixes #4293.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

driver: add support for a more generic diagnostic handler #4302

driver: add support for a more generic diagnostic handler #4302

ljmf00 commented Jan 22, 2023 •

edited

ljmf00 commented Jan 22, 2023

thewilsonator commented Jan 22, 2023

ljmf00 Jan 22, 2023

JohanEngelen Jan 22, 2023

ljmf00 commented Jan 22, 2023

JohanEngelen Jan 22, 2023

ljmf00 Jan 22, 2023

JohanEngelen Jan 22, 2023

JohanEngelen Jan 22, 2023

JohanEngelen Jan 22, 2023 •

edited

ljmf00 Jan 22, 2023

JohanEngelen Jan 22, 2023

JohanEngelen commented Jan 22, 2023 •

edited

ljmf00 commented Jan 22, 2023

ljmf00 commented Jan 22, 2023

kinke commented Jan 22, 2023

JohanEngelen commented Jan 22, 2023

JohanEngelen Jan 22, 2023

ljmf00 commented Jan 23, 2023


		import ldc.attributes;

		// CHECK: {{.*}}Warning: stack frame size (8) exceeds limit (1) in '_D20attr_warn_stack_size6foobarFiZf'


		// REQUIRES: atleast_llvm1400

		// RUN: %ldc -wi -c %s 2>&1 \| FileCheck %s

driver: add support for a more generic diagnostic handler #4302

Are you sure you want to change the base?

driver: add support for a more generic diagnostic handler #4302

Conversation

ljmf00 commented Jan 22, 2023 • edited

ljmf00 commented Jan 22, 2023

thewilsonator commented Jan 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljmf00 commented Jan 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanEngelen Jan 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanEngelen commented Jan 22, 2023 • edited

ljmf00 commented Jan 22, 2023

ljmf00 commented Jan 22, 2023

kinke commented Jan 22, 2023

JohanEngelen commented Jan 22, 2023

Choose a reason for hiding this comment

ljmf00 commented Jan 23, 2023

ljmf00 commented Jan 22, 2023 •

edited

JohanEngelen Jan 22, 2023 •

edited

JohanEngelen commented Jan 22, 2023 •

edited