Page MenuHomePhabricator

[analyzer] Fix SARIF column locations
ClosedPublic

Authored by jranieri-grammatech on Nov 25 2019, 11:29 AM.

Details

Summary

This fixes SARIF column locations to be end-exclusive and be counted in terms of Unicode characters.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptNov 25 2019, 11:29 AM
aaron.ballman added inline comments.Dec 11 2019, 10:45 AM
clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
31

You don't really need the whole preprocessor, just the language options, right? If so, why not thread the language options through rather than the PP?

150

Can you add some text to the assertion to explain what's gone wrong if the assert triggers? e.g. assert(!Loc.isInvalid() && "invalid location when adjusting column position"); or something (same for the others asserts you've added)?

clang/test/Analysis/diagnostics/sarif-multi-diagnostic-test.c
34

Formatting of the curly brace looks wrong.

35–36

What file encoding is used by this file (and is there a BOM)? Have you tried this test on multiple platforms by any chance?

Addressed review comments.

jranieri-grammatech marked 4 inline comments as done.Dec 20 2019, 1:32 PM
jranieri-grammatech added inline comments.
clang/test/Analysis/diagnostics/sarif-multi-diagnostic-test.c
35–36

The file is encoded in UTF-8 without a BOM. I have not tried the test on other platforms.

aaron.ballman marked an inline comment as done.

Mostly LG aside from a question about an assertion. Adding other reviewers outside of GrammaTech in case there are other concerns we've missed.

clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
157

I don't think this API is able to return a null buffer, but it can return an invalid buffer under legitimate circumstances, I believe. For instance, if the source location is in the scratch space used for things like macros defined on the command line with -D. I think you should pass the optional invalid parameter to the call to getBuffer() and then do no adjustments if the buffer is invalid. WDYT?

clang/test/Analysis/diagnostics/sarif-multi-diagnostic-test.c
35–36

Okay, I think that's reasonable, thanks!

jranieri-grammatech marked an inline comment as not done.Dec 23 2019, 10:25 AM
jranieri-grammatech added inline comments.
clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
157

Good catch that this can't return NULL.

I tested out defining things on the command line, but this code deals with expansion locations and wasn't affected by it. Unless you can think of a way to reproduce it, I think I'd rather pass in the invalid argument and assert that it isn't invalid.

aaron.ballman accepted this revision.Jan 6 2020, 11:22 AM

LGTM! I think we've had sufficient time for other reviewers to lodge concerns and we can deal with any other issues post-commit. Do you need me to commit on your behalf?

clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
157

I think I'd rather pass in the invalid argument and assert that it isn't invalid.

That's fine by me -- I can't think of another way to reproduce it.

This revision is now accepted and ready to land.Jan 6 2020, 11:22 AM
This revision was automatically updated to reflect the committed changes.