Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ljmf00
Copy link
Contributor

@ljmf00 ljmf00 commented Jan 22, 2023

Signed-off-by: Luís Ferreira [email protected]


Fixes #4293.

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
Copy link
Contributor Author

ljmf00 commented Jan 22, 2023

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

@thewilsonator
Copy link
Contributor

Do you have a test case for this?

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

import ldc.attributes;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

ljmf00 commented Jan 22, 2023

Do you have a test case for this?

From my PR description :)

I'm going to add tests for this tomorrow.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@JohanEngelen JohanEngelen Jan 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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 :/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@JohanEngelen
Copy link
Member

JohanEngelen commented Jan 22, 2023

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.

@ljmf00 ljmf00 force-pushed the use-warning-stack-size-diag branch 2 times, most recently from a539f13 to 2e943f8 Compare January 22, 2023 12:51
@ljmf00 ljmf00 force-pushed the use-warning-stack-size-diag branch from 2e943f8 to 2fe9396 Compare January 22, 2023 13:00
@ljmf00
Copy link
Contributor Author

ljmf00 commented Jan 22, 2023

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
Copy link
Contributor Author

ljmf00 commented Jan 22, 2023

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

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

kinke commented Jan 22, 2023

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

@JohanEngelen
Copy link
Member

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.


// REQUIRES: atleast_llvm1400

// RUN: %ldc -wi -c %s 2>&1 | FileCheck %s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

ljmf00 commented Jan 23, 2023

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.

kinke added a commit to kinke/ldc that referenced this pull request Feb 26, 2023
kinke added a commit to kinke/ldc that referenced this pull request Feb 26, 2023
kinke added a commit to kinke/ldc that referenced this pull request Feb 26, 2023
kinke added a commit to kinke/ldc that referenced this pull request Feb 26, 2023
kinke added a commit that referenced this pull request Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Errors in inline assembly don't stop compilation
4 participants