Need help designing a new diagnostic formatting command line option for use with x86_64-pc-win32

Andrew_Fish · May 18, 2011, 7:03am

For EFI firmware we use x86_64-pc-win32-darwin, but the default behavior for diagnostics on this triple is to suppress column number printing, and format line numbers to be compatible with Visual Studio. This breaks our Xcode IDE, and command line yank put. Attached is my first take at a patch to fix this. IMHO the default diagnostic output for clang, should be clangs native diagnostic output that matches the documentation. I understand the need to modify the output to match the IDE that is being used, as I’m fixing that reverse problem. It would seem OK to add a flag, like -fno-diagnostic-standard-format, when clang is being used with an incompatible IDE.

**-f[no-]diagnostics-standard-format**: Print diagnostics in standard clang format, or a non standard OS specific format.

This option, which defaults to on, controls whether or not Clang uses the default diagnostic output format, or use an OS specific diagnostic format. For example, when this is disabled for the x86_64-pc-win32 triple, Clang will modify diagnostic output something like:

  test.c(2): error: unknown type name 'asdkjf'

  asdkjf

Note: This changes the default behavior today so it would look like this (normal clang):

clang -ccc-host-triple x86_64-pc-win32 ve.c -S
ve.c:2:1: error: unknown type name ‘asdkjf’
asdkjf
^

To get the old behavior you need to add the new flag:

clang -ccc-host-triple x86_64-pc-win32 ve.c -S -fno-diagnostics-standard-format
ve.c(2) : error: unknown type name ‘asdkjf’
asdkjf
^

If I was doing this from scratch I would probably add a flag per IDE that was not compatible with the clang diagnostics format that would modify the clang diagnostic output to be compatible for that IDE. So something like -fdiagnostics-ide=VisualStudio or -fdiagnostics-vi. I picked --fno-diagnostic-standard-format to better match what the currently code was doing:

if (LangOpts && LangOpts->Microsoft) {

Given I’m a novice at clang, llvm, and C++ I figured changing as little as possible was a good plan.

Andrew Fish

Patch to svn r131522.

fno_diagnostic_standard_format.patch (4.61 KB)

Andrew_Fish · May 18, 2011, 3:51pm

PS -fdiagnostics-ide=vi looks like:
ve.c: +2: error: unknown type name 'asdkjf'
asdkjf
^

Sorry about the typo!

ve.c +2: error: unknown type name 'asdkjf'

You want to be able to yank put to the tool command line so it would be 'vi ve.c +2'.

Thanks,

Andrew

Doug_Gregor · May 18, 2011, 3:52pm

Hi Andrew,

For EFI firmware we use x86_64-pc-win32-darwin, but the default behavior for diagnostics on this triple is to suppress column number printing, and format line numbers to be compatible with Visual Studio. This breaks our Xcode IDE, and command line yank put. Attached is my first take at a patch to fix this. IMHO the default diagnostic output for clang, should be clangs native diagnostic output that matches the documentation. I understand the need to modify the output to match the IDE that is being used, as I’m fixing that reverse problem. It would seem OK to add a flag, like -fno-diagnostic-standard-format, when clang is being used with an incompatible IDE.

**-f[no-]diagnostics-standard-format**: Print diagnostics in standard clang format, or a non standard OS specific format.
This option, which defaults to on, controls whether or not Clang uses the default diagnostic output format, or use an OS specific diagnostic format. For example, when this is disabled for the x86_64-pc-win32 triple, Clang will modify diagnostic output something like:
  test.c(2): error: unknown type name 'asdkjf' 
  asdkjf
  ^
Note: This changes the default behavior today so it would look like this (normal clang):
clang -ccc-host-triple x86_64-pc-win32 ve.c -S
ve.c:2:1: error: unknown type name ‘asdkjf’
asdkjf
^

To get the old behavior you need to add the new flag:

clang -ccc-host-triple x86_64-pc-win32 ve.c -S -fno-diagnostics-standard-format
ve.c(2) : error: unknown type name ‘asdkjf’
asdkjf
^

If I was doing this from scratch I would probably add a flag per IDE that was not compatible with the clang diagnostics format that would modify the clang diagnostic output to be compatible for that IDE. So something like -fdiagnostics-ide=VisualStudio or -fdiagnostics-vi.

I think this is the superior design, with something like -fdiagnostics-format=msvc used to indicate how we should format diagnostics. It’ll scale if we end up having to support some other display format for a different IDE or compiler.

I picked --fno-diagnostic-standard-format to better match what the currently code was doing:

if (LangOpts && LangOpts->Microsoft) {

The current code is really sketchy, since it conflates a language-parsing option with a diagnostic-display option, when these should be disjoint subsystems.

Given I’m a novice at clang, llvm, and C++ I figured changing as little as possible was a good plan.

That’s understandable, and your patch looks quite good. The only change I would have requested would be to add “-fdiagnostic-standard-format” as well as “-fno-diagnostic-standard-format”, because then the driver can be taught how to take the last flag from the command line, e.g.,

clang -fno-diagnostic-standard-format t.c -fdiagnostic-standard-format

However, I think the best way forward is to implement -fdiagnostics-format=xxx, where “xxx” is either clang (the default) or msvc (Visual Studio-style). If the -fdiagnostics-format=xxx option isn’t provided to the driver, the driver will pick based on the target triple, so that non-EFI MSVC targets get “msvc” and other targets get “clang”.

Doug

Ted_Kremenek · May 18, 2011, 6:24pm

Hi Andrew, Doug,

I’m wondering if support for Microsoft-style diagnostics should be implemented as a new DiagnosticClient (e.g., MSVCTextDiagnosticPrinter). I’m not certain what the plan was, but putting this all into TextDiagnosticPrinter seems a little gross to me. With a separate DiagnosticClient, we might not need extra bits in DiagnosticOptions.

Just my $0.02.

Ted

Andrew_Fish · May 18, 2011, 7:33pm

I’m working on a patch to implement -fdiagnostic-format=xxx per Doug’s suggestion, and will post the patch to mailing list for review shortly.

I implemented for msvc and vi variants, here are the changes in TextDiagnosticPrinter.cpp for TextDiagnosticPrinter::HandleDiagnostic()

if (DiagOpts->Format == 1)
// msvc parsable format
OS << PLoc.getFilename() << ‘(’ << LineNo << ") : “;
else if (DiagOpts->Format == 2)
// vi command line format
OS << PLoc.getFilename() << " +” << LineNo << ‘:’;
else
// clang format
OS << PLoc.getFilename() << ‘:’ << LineNo << ‘:’;
if (DiagOpts->ShowColumn)
if (unsigned ColNo = PLoc.getColumn())
OS << ColNo << ‘:’;

Where the original code was:

// Emit a Visual Studio compatible line number syntax.
if (LangOpts && LangOpts->Microsoft) {
OS << PLoc.getFilename() << ‘(’ << LineNo << ‘)’;
OS << " : ";
} else {
OS << PLoc.getFilename() << ‘:’ << LineNo << ‘:’;
if (DiagOpts->ShowColumn)
if (unsigned ColNo = PLoc.getColumn())
OS << ColNo << ‘:’;
}

Seems like a bit of overkill to implement a new DiagnosticClient for one line of code? Maybe some method for abstracting filename, line number, and column printing would be better? Then again my brain thinks in C and not C++.

Thanks,

Andrew

Ted_Kremenek · May 18, 2011, 7:41pm

I agree; if this is the only difference, then a new DiagnosticClient is overkill.

Doug_Gregor · May 18, 2011, 7:48pm

Hi Andrew, Doug,

I’m wondering if support for Microsoft-style diagnostics should be implemented as a new DiagnosticClient (e.g., MSVCTextDiagnosticPrinter). I’m not certain what the plan was, but putting this all into TextDiagnosticPrinter seems a little gross to me. With a separate DiagnosticClient, we might not need extra bits in DiagnosticOptions.

I’m working on a patch to implement -fdiagnostic-format=xxx per Doug’s suggestion, and will post the patch to mailing list for review shortly.

I implemented for msvc and vi variants, here are the changes in TextDiagnosticPrinter.cpp for TextDiagnosticPrinter::HandleDiagnostic()

if (DiagOpts->Format == 1)
// msvc parsable format
OS << PLoc.getFilename() << ‘(’ << LineNo << ") : “;
else if (DiagOpts->Format == 2)
// vi command line format
OS << PLoc.getFilename() << " +” << LineNo << ‘:’;
else
// clang format
OS << PLoc.getFilename() << ‘:’ << LineNo << ‘:’;
if (DiagOpts->ShowColumn)
if (unsigned ColNo = PLoc.getColumn())
OS << ColNo << ‘:’;

I’d rather that DiagOpts->Format use an enumeration type, so this can be a switch() on the format. Otherwise, this looks fine.

Where the original code was:

// Emit a Visual Studio compatible line number syntax.
if (LangOpts && LangOpts->Microsoft) {
OS << PLoc.getFilename() << ‘(’ << LineNo << ‘)’;
OS << " : ";
} else {
OS << PLoc.getFilename() << ‘:’ << LineNo << ‘:’;
if (DiagOpts->ShowColumn)
if (unsigned ColNo = PLoc.getColumn())
OS << ColNo << ‘:’;
}

Seems like a bit of overkill to implement a new DiagnosticClient for one line of code? Maybe some method for abstracting filename, line number, and column printing would be better? Then again my brain thinks in C and not C++.

I agree that it would be overkill to create a new DiagnosticClient. Since the variation between the formats isn’t so large now, this doesn’t need to be abstracted further unless the code is repeated elsewhere.

Doug

Andrew_Fish · May 18, 2011, 8:32pm

I implemented for msvc and vi variants, here are the changes in TextDiagnosticPrinter.cpp for TextDiagnosticPrinter::HandleDiagnostic()

if (DiagOpts->Format == 1)
// msvc parsable format
OS << PLoc.getFilename() << ‘(’ << LineNo << ") : “;
else if (DiagOpts->Format == 2)
// vi command line format
OS << PLoc.getFilename() << " +” << LineNo << ‘:’;
else
// clang format
OS << PLoc.getFilename() << ‘:’ << LineNo << ‘:’;
if (DiagOpts->ShowColumn)
if (unsigned ColNo = PLoc.getColumn())
OS << ColNo << ‘:’;

I’d rather that DiagOpts->Format use an enumeration type, so this can be a switch() on the format. Otherwise, this looks fine.

I copied DiagnosticOptions ShowCategories and it did not implement an enum. I was trying to copy code as much as I could to follow the coding standards. What would a good name for the enum be?

Thanks,

Andrew

Doug_Gregor · May 18, 2011, 8:40pm

TextDiagnosticFormat?

Doug

dblaikie · May 18, 2011, 9:04pm

Did you mean to also reply to cfe-dev?

Yes indeed, sorry for the noise. I hope you don’t mind my adding them back on here.