Page MenuHomePhabricator

Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.
ClosedPublic

Authored by eric_niebler on May 2 2016, 5:26 PM.

Diff Detail

Event Timeline

eric_niebler retitled this revision from to Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths..
eric_niebler updated this object.
eric_niebler added a reviewer: bruno.
eric_niebler added a subscriber: cfe-commits.

Some minor remarks. Sorry for being finicky :).

include/clang/Basic/VirtualFileSystem.h
97

No else needed after return.

lib/Lex/PPDirectives.cpp
1673

May you create a bool variable for this long condition and name it so that it's clear what it checks?

1678–1700

Could you extract a function for this chunk? Something like bool SuggestReplacement = simplifyPath(Components);.
The HandleIncludeDirective method is already loooong enough.

eric_niebler added inline comments.May 18 2016, 11:40 AM
include/clang/Basic/VirtualFileSystem.h
97

But then Status is not in scope.

Factor out TrySimplifyPath from Preprocessor::HandleIncludeDirective. Other review feedback.

Nice job! Thanks for taking my remarks into account.

include/clang/Basic/VirtualFileSystem.h
14

Oops, my fault.

eric_niebler marked 2 inline comments as done.May 18 2016, 3:34 PM

Thanks for the feedback, @curdeius. What happens now? Do I just wait until somebody accepts the llvm diff and this one? How do I increase the likelihood that that happens?

rsmith added a subscriber: rsmith.May 19 2016, 5:38 PM

Thanks for this! Sorry for the delay on the review side. Generally, the approach here looks fine, and I don't have any high-level concerns beyond a performance concern over whatever dark magic you're doing on the LLVM side to get this filename.

Please add some tests for the FixItHint replacement text. See test/FixIt for examples of how to do this.

I've left a handful of mostly-stylistic comments inline.

include/clang/Basic/DiagnosticLexKinds.td
278

Diagnostics should start with a lowercase letter and not end in a period.

lib/Lex/PPDirectives.cpp
1503–1504

{ on previous line, please.

1508

& on the right.

1510–1511

Local variable names should start with an uppercase letter.

1514

& on the right.

1516–1519

Hmm. A .. doesn't necessarily remove the prior path component, especially in the presence of directory symlinks. I suspect it's not worth the trouble of trying to get this right, but a comment saying that this is a best-effort attempt to handle ..s would be useful.

1704–1705

Checking whether a warning is enabled can be surprisingly expensive; TrySimplifyPath might actually be cheaper.

test/Lexer/case-insensitive-include.c
5

Please explicitly cd to somewhere around %T here rather than assuming you know where it is relative to the current directory; we do not guarantee which directory the test will be run from and it does differ in practice between different ways of running the tests.

9–12

What's the reason for this test including this file twice?

Please add some tests for the FixItHint replacement text. See test/FixIt for examples of how to do this.

Thanks for the suggestion. After adding tests for this, I noticed badness with the path separators in the FixIt hint when running on Windows. I'm working on a fix.

Add fixit tests, fix path separator fixit issue on Windows.

rsmith accepted this revision.May 27 2016, 3:04 PM
rsmith added a reviewer: rsmith.

LGTM once the LLVM side is landed.

include/clang/Basic/DiagnosticLexKinds.td
278

The fix-it hints aren't shown in some modes of operation; it might be helpful for this diagnostic to be a bit more explicit about what's non-portable since without the fixit it's not obvious. Maybe something like

"non-portable path to file '%0'; specified path differs in case from file name on disk"

where %0 is the corrected path?

lib/Lex/PPDirectives.cpp
1763–1765

Looks like this comment is stale?

This revision is now accepted and ready to land.May 27 2016, 3:04 PM

Awesome, thanks.

include/clang/Basic/DiagnosticLexKinds.td
278

Good suggestion.

lib/Lex/PPDirectives.cpp
1763–1765

Oops, yeah.

eric_niebler edited edge metadata.

Remove stale comment, tweak for diagnostic wording.

twoh closed this revision.Jun 3 2016, 12:31 PM
twoh added a subscriber: twoh.

I have commit in r271708: http://reviews.llvm.org/rL271708

twoh added a comment.Jun 3 2016, 8:46 PM

Reverted in r271761.

eric_niebler updated this object.

Rework the patch to only warn by default for include files not found in system include directories, unless they are known "standard" headers that should be portable (C/C++/Posix/Boost). Add an additional warning -Wnonportable-system-include-path for turning the warning on for user code that #include's other system headers like <Windows.h>. (As always, warning within system headers are suppressed.)

eric_niebler reopened this revision.Jun 8 2016, 10:15 AM
eric_niebler added a subscriber: bogner.

Reopening. I would like some eyes on the updated patch before we merge. @rsmith Would you prefer this (and D19842) in new diffs?

@bruno, @bogner: This should fix the use-after-free that ASAN was complaining about. ASAN rules.

This revision is now accepted and ready to land.Jun 8 2016, 10:15 AM
bruno edited edge metadata.Jun 8 2016, 1:54 PM

Before this goes in again, I want to double check that this doesn't affect compile time on darwin + frameworks. Will get back to you asap.

lib/Lex/PPDirectives.cpp
218

-> "char &Ch"

1666

-> "for (auto..."

rsmith added a comment.Jun 8 2016, 2:28 PM

Looks OK to me, pending Bruno's confirmation that performance is acceptable.

lib/Lex/PPDirectives.cpp
102

Can you use llvm::StringSwitch for this? I'd be interested to see how the performance compares.

eric_niebler added inline comments.Jun 8 2016, 2:41 PM
lib/Lex/PPDirectives.cpp
33

You mean, instead of the StringSet below? Looks like StringSwitch just turns into cascading memcmp's. Bet I can tell you how that performs versus a hash set. :-) Also, with the StringSet, I get to initialize it once and reuse it many times. I expect that will be pretty darn quick at runtime, but I'm looking forward to @bruno's results.

rsmith added inline comments.Jun 8 2016, 3:41 PM
lib/Lex/PPDirectives.cpp
33

Right, I'm not suggesting StringSwitch will be faster; it's preferable for other reasons (it avoids the memory and shutdown costs of the static local set). We should stick with what you have if the performance advantage is measurable; otherwise my preference would be to use StringSwitch. But it's only a slight preference -- if you'd rather not, I won't complain.

[StringSwitch isn't /quite/ as bad as you're suggesting: it always first compares on length, and it typically compiles into a switch on string length followed by memcmps. Moreover, the code should be "obvious" enough that compilers can (at least in principle) optimize those memcmps very aggressively, right down into the equivalent of an unrolled DFA or a perfect hash function, but I'm not at all confident that LLVM will actually do that =)]

220

Comment doesn't match code: the ASCII range ends at 0x7F.

225–229

Rather than hardcoding this platform-specific test here, maybe use llvm::sys::path::is_separator(Ch).

eric_niebler added inline comments.Jun 8 2016, 3:41 PM
lib/Lex/PPDirectives.cpp
220

This should be 0x7F. I'll fix it.

bruno added inline comments.Jun 8 2016, 4:55 PM
lib/Lex/PPDirectives.cpp
179

Applying your patch reveled a bunch of ^M (carriage-return) characters in the C++ library headers section above. Also make sure to clean this up.

Before this goes in again, I want to double check that this doesn't affect compile time on darwin + frameworks.

@bruno, you're not likely to find a difference for darwin + frameworks since the frameworks headers like Cocoa/Cocoa.h don't exist on-disk with that path -- at least not on my machine. As a result, the attempt to get the "real path name" fails and no diagnostic is issued; the code path you want to measure will never be hit. This appears to be a shortcoming of the way I'm trying to get the true case of the files as they are opened. Correctly handing this would greatly increase the complexity of the patch. Apple == :-(

eric_niebler added inline comments.Jun 9 2016, 4:20 PM
lib/Lex/PPDirectives.cpp
33

@rsmith wrote:

We should stick with what you have if the performance advantage is measurable; otherwise my preference would be to use StringSwitch.

I tried StringSwitch on Windows.h where there are lots of wrongly-cased #includes. I couldn't measure the difference. I'll update the diff.

eric_niebler edited edge metadata.

Replace StringSet with StringSwitch, ASCII range ends at 0x7f not 0xff, miscellaneous formatting tweaks.

rsmith added a comment.Jun 9 2016, 5:38 PM

Thanks for the updates, LGTM (@bruno, did you get the performance numbers you wanted?)

@rsmith wrote:

Thanks for the updates, LGTM (@bruno, did you get the performance numbers you wanted?)

FWIW, I benchmarked (on Windows, where problems are likely to occur) with 3 implementations of warnByDefaultOnWrongCase: 1) StringSwitch, 2) StringSet, and 3) noop. I found no measurable performance difference between any of them. This code path just isn't hot enough to matter.

If I don't hear back, maybe @twoh will merge these diffs again by EOD. And hopefully we'll hear fewer screams this time. :-)

eric_niebler reopened this revision.Jun 13 2016, 12:00 PM

Looks like I need s/std::size_t/size_t/ to keep the win2008 buildbot happy. Also, Taewook and I are looking into the clang-format test failure, but my suspicion is that that one isn't ours.

This revision is now accepted and ready to land.Jun 13 2016, 12:00 PM

FormatTests unittest passes for me with just my patch. Not sure which diff is causing that failure but it seems not to be mine.

Shall Taewook just commit the s/std::size_t/size_t/ change, or does something else need to be done (revert & resubmit)?

twoh added a comment.Jun 13 2016, 12:19 PM

I already reverted the path in http://reviews.llvm.org/rL272572, so re-commit should be the way to go.

s/std::size_t/size_t/ to keep the Win2008 buildbot happy.

Win2008 buildbot still unhappy. Sorry about this guys. I suppose it's the constexpr. We don't have a win2008 machine locally to test on. Is there a way for us to run this through your gauntlet without actually doing a commit?

s/constexpr/const/

FTR, I finally test it out for compile time and could not notice any difference besides noise. Thanks Eric!