Page MenuHomePhabricator

pr21217 - -fmodule-map-file should warn on non-existent file
Needs ReviewPublic

Authored by jtsoftware on Nov 19 2014, 11:00 AM.

Details

Reviewers
rsmith
Summary

Add warning check for missing module map file specified with -fmodule-map-file. Hopefully I got the warning put in right. Please review.

Diff Detail

Event Timeline

jtsoftware retitled this revision from to pr21217 - -fmodule-map-file should warn on non-existent file.
jtsoftware updated this object.
jtsoftware edited the test plan for this revision. (Show Details)
jtsoftware added a reviewer: silvas.
jtsoftware added a reviewer: rsmith.
jtsoftware added a subscriber: Unknown Object (MLST).

Looks like this loop dates to a commit by djasper. Adding him as a reviewer.

jtsoftware updated this object.Nov 19 2014, 12:33 PM
jtsoftware edited edge metadata.
djasper edited edge metadata.Nov 20 2014, 2:06 AM

This basically looks good to me, but I'd like Richard to take a look, too.

include/clang/Basic/DiagnosticLexKinds.td
617

I'd phrase it as:

"File '%0' specified via -fmodule-map-file not found"
test/Modules/pr21217.cpp
7–9

Do these serve any purpose?

jtsoftware edited edge metadata.

Sean, thanks for adding Daniel.

Daniel, thanks for the review. I've updated accordingly per your comments. But note that I had to have at least one include in the test, or the check is not performed. It seems like this should be okay, since any code using modules will have some include or import. Otherwise I would need to do the check earlier when options are processed.

rsmith edited edge metadata.Nov 20 2014, 2:55 PM

(Question for Daniel:) Why do we defer loading the explicitly-specified module map files until we perform a lookup? It seems like that will not work for Objective-C, where a module can be imported without ever looking up a file (by using @import). As John notes, this also means that there can be corner cases where we don't diagnose a missing module map file that was specified on the command line.

include/clang/Basic/DiagnosticLexKinds.td
616–618

I think this should be an error rather than a warning. I don't think there are any other cases where we ignore a missing input file that was specified on the command line. Also, diagnostic messages should start with a lowercase letter.

jtsoftware updated this revision to Diff 16512.Nov 21 2014, 2:54 PM
jtsoftware edited edge metadata.

Moved loading of -fmodule-map-file files to HeaderSearch initialization in Preprocessor initialization.
Changed diagnostic to an error.

rsmith added inline comments.Nov 21 2014, 5:00 PM
include/clang/Basic/DiagnosticLexKinds.td
554

Diagnostic messages should start with a lowercase letter.

lib/Lex/HeaderSearch.cpp
73–76

Please add braces and indent.

77

This seems unnecessary now you're doing it in HeaderSearch initialization.

jtsoftware updated this revision to Diff 16591.Nov 24 2014, 6:00 PM

Sorry, I missed the hard tab, braces, and etc. However, if I remove the clearing of the ModuleMapFiles member, one of the tests, Modules/modular_maps.cpp segfaults deep in some AST code, but only on running the test on Linux, so it's kind of hard to debug, not being much of a Linux/gdb guy. Did I put this code in the right place? I was kind of uncomfortable putting it in the HeaderSearch constructor, but it is a convenient place, since HeaderSearch has convenient members. I don't see any other references to the ModuleMapFiles member, so I'm not sure what triggers the segfault only on Linux, unless it's some memory-stomping or post-free-write issue somewhere.

rsmith added a comment.Dec 5 2014, 5:14 PM

I played with this some more, and determined that the best place to load
the module map files is from the frontend rather than from header search; I
modified your patch to do that and landed it as r223561.

djasper resigned from this revision.Apr 12 2015, 1:23 AM
djasper removed a reviewer: djasper.
silvas resigned from this revision.Jul 8 2016, 11:39 PM
silvas removed a reviewer: silvas.