[SR-2402][ClangImporter] Import nullary function macros #6530

modocache · 2017-01-03T03:15:52Z

Import function-like macros that take no arguments, assuming their bodies are something ClangImporter would otherwise be able to import.

Modify shouldIgnoreMacro such that it only ignores function-like macros if they take more than zero arguments.
ClangImporter's macro importing normally creates VarDecl. Modify that to create FuncDecl if the original macro is function-like.

The implementation might be gross, I'm not sure. Another problem: importing macros from another module doesn't really work. Does anyone know what I'm missing here?

CodaFi

Great job! A couple quick comments.

CodaFi · 2017-01-03T03:25:51Z

lib/ClangImporter/ImportDecl.cpp

+
+  // Finally, set the body.
+  func->setBody(BraceStmt::create(C, SourceLoc(),
+                                  ASTNode(ret),


Nit: This should implicitly convert to ASTNode. I think you want to make the array explicit

func->setBody(BraceStmt::create(C, SourceLoc(), { ret }, SourceLoc()));

CodaFi · 2017-01-03T03:28:40Z

lib/ClangImporter/ImportName.cpp

    return true;

-  // Consult the blacklist of macros to suppress.
+  // Consult the list of macros to suppress.


Any reason why this is changed?

"Blacklist" is considered exclusionary in some contexts. Here it seems "list" conveys the same meaning without any unintended consequences. I was already modifying the body of this method, so I thought I'd include this comment change as well.

CodaFi · 2017-01-03T03:32:29Z

lib/ClangImporter/ImportDecl.cpp

 }

+ValueDecl *
+ClangImporter::Implementation::createFunction(Identifier name, DeclContext *dc,


Since this code is pulled from ClangImporter::Implementation::createConstant, could you find a way to factor its body to call this?

Will do, thanks!

modocache · 2017-01-03T14:16:01Z

Thanks for the review, @CodaFi! 🙇

jrose-apple · 2017-01-04T19:00:24Z

This is pretty much the approach I was going with in my first attempt, so it looks good to me. The trouble you're having with functions chaining to macros is because of ClangImporter.cpp:390: "If it's an identifier that is itself a macro, look into that macro." That is, importMacro is recursive, but you aren't keeping track of whether the outer thing is a function.

Also worried now we're going to run into the same kind of ambiguity issue as #6531—if people have been working around this in overlays, we're in trouble. I suppose we could limit this to Swift 4 mode and cross our fingers.

Don't forget to add a test for function-like macros invoking function-like macros and object-like macros invoking function-like macros.

modocache · 2017-01-04T19:33:12Z

This is pretty much the approach I was going with in my first attempt

Oh, I'm sorry if I duplicated your work here, @jrose-apple. Was your first attempt in a pull request somewhere? I'm sorry if I missed it.

Don't forget to add a test for function-like macros invoking function-like macros and object-like macros invoking function-like macros.

Will do!

jrose-apple · 2017-01-04T19:38:32Z

Oh, no, I had a partial patch from a long time ago (over a year) that I never attached to the JIRA because it didn't apply cleanly. Actually, I vaguely remember there was a problem when you actually tried to IRGen a call to one of these macros, so please include at least one execution test as well!

modocache · 2017-01-04T19:53:08Z

I did confirm locally that a call to a function-like macro succeeded in a running program, but I'll certainly add a test. Thanks for the suggestion!

slavapestov · 2017-01-04T22:28:26Z

I wonder if we need a general solution to deal with potentially source-breaking clangimporter changes like these.

jrose-apple · 2017-01-04T22:35:43Z

Officially the Clang importer is part of the language and major changes should go through swift-evolution. Things like this are on the border of "major change", though.

In that sense, limiting this to Swift 4 mode is probably the correct answer, but that doesn't help cross-language compilation (i.e. Swift 4 code trying to use both Swift 3 code and imported Objective-C code).

slavapestov · 2017-01-09T08:51:12Z

@swift-ci Please test

slavapestov · 2017-01-09T08:51:22Z

@swift-ci Please test

swift-ci · 2017-01-09T09:17:59Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - c85005766a0b47220fc776697d4d3fee073f3aae
Test requested by - @slavapestov

modocache · 2017-01-09T14:28:35Z

(Just FYI, I plan on making the changes Jordan suggested and then getting this merged before the end of this week. I haven't forgotten about it! 😅 )

jrose-apple · 2017-01-09T17:07:19Z

I think given Doug's response to #6531 it's okay to leave this on even in Swift 3 mode. We'll handle conflicts if and when they happen, under the expectation that providing the same API in an overlay would be rare.

slavapestov · 2017-01-11T02:17:45Z

@modocache Do you mind adding an entry to CHANGLEOG.md as well? We have several new ClangImporter features that might affect users so I'm following up with PR authors about documenting them.

modocache · 2017-01-12T17:13:10Z

Uploaded some changes, but still more to come.

TODO

Add CHANGELOG.md entry
Respond to code review comments
Keep track of whether outer macro is function-like in recursive importMacro call, in order to get the USES_MACRO_FROM_OTHER_MODULE_1_FUNC() tests working
Add a test that runs an executable that references a function-like macro, to ensure no bugs occur in IRGen or what not
~~Add a test for function-like macros invoking function-like macros and object-like macros invoking function-like macros~~ Add a test that demonstrates macros invoking function-like macros are not imported

modocache · 2017-01-12T18:07:02Z

Add a test for function-like macros invoking function-like macros and object-like macros invoking function-like macros

@jrose-apple, I thought about this a bit more, and I wonder if it makes sense to punt this to a future pull request. I don't think the ClangImporter's macro code has any logic to handle a call expression, i.e.: #define foo bar() or #define baz() bork(), does it? It seems like supporting the ability to import nullary function-like macros is tangential to supporting macros that call other macros or functions.

Of course, I'd love to import any kind of macro, but I'm worried about biting off more than I can chew. If I can reduce the scope of this pull request I'd happily do so. Thoughts? 🙌

jrose-apple · 2017-01-12T18:20:43Z

I think it's fine to save that feature for later, but I'd like the tests there to be sure we're not mis-importing them somehow.

modocache · 2017-01-12T18:29:14Z

Sure thing. Right now we don't import them at all, I think the ClangImporter gives up because of the call expression on the right. I'll add a test case that demonstrates.

modocache · 2017-01-12T20:20:57Z

@swift-ci please smoke test

jrose-apple · 2017-01-12T21:10:22Z

lib/ClangImporter/ImportMacro.cpp

-      return importLiteral(impl, DC, macro, name, tok, ClangN, castType);
+      return importLiteral(
+          impl, DC, macro, name, tok, ClangN, castType,
+          isOriginalMacroFunctionLike || macro->isFunctionLike());


I think this is incorrect. It's only the original macro that matters, no?

Absent any recursion, importLiteral is called immediately. Since isOriginalMacroFunctionLike is false by default, this means that #define FOO() 1 would be imported as a global variable, not a Swift function. So I think this is correct as is. Modifying it to only use isOriginalMacroFunctionLike causes the newly added function-like tests to fail.

Hm, okay. I'm suspicious of cases like #define BAR FOO() but we can wait until that actually happens to fix this.

jrose-apple · 2017-01-12T21:12:06Z

test/ClangImporter/macro-functions.swift

+// these can be used in executables, and don't crash at runtime.
+
+// RUN: rm -rf %t && mkdir -p %t
+// RUN: %target-swiftc_driver %s -import-objc-header %S/Inputs/macro-functions.h -o %t/macro-functions


Please use %target-swift-build and %target-run for this, so that it works properly on iOS, and please add REQUIRES: execution_test so that it isn't run for cross-compilation cases. The test/Interpreter/ directory is probably a more appropriate place to put the test, too; we tend to cluster execution tests there.

modocache · 2017-01-13T00:00:38Z

@swift-ci please smoke test

jrose-apple · 2017-01-13T00:49:50Z

test/ClangImporter/macro-functions.swift

@@ -0,0 +1,13 @@
+// Other ClangImporter tests verify that function-like C macros are imported
+// into Swift so that identifiers are resolved correclty. This test verifies


Typo: "correclty".

modocache · 2017-01-13T01:32:21Z

I'm sure it's due to the changes I've made here, since this doesn't reproduce using master, but the macOS error is pretty wacky. Basically, when running the Swift REPL, this happens:

$ swift
***  You are running Swift's integrated REPL,  ***
***  intended for compiler and stdlib          ***
***  development and testing purposes only.    ***
***  The full REPL is built as part of LLDB.   ***
***  Type ':help' for assistance.              ***
(swift) import MapKit
(swift) foo
<REPL Input>:1:1: error: use of unresolved identifier 'foo'
foo
^~~
<unknown>:0: warning: 'cacheParamsComputed' is deprecated
<unknown>:0: warning: 'cacheAlphaComputed' is deprecated
<unknown>:0: warning: 'keepCacheWindow' is deprecated
<unknown>:0: error: 'memoryless' is unavailable
Metal.MTLCommandBufferError:55:14: note: 'memoryless' has been explicitly marked unavailable here
        case memoryless
             ^

Same thing happens when I run:

import Metal
foo

I'm looking into why this might be happening.

gparker42 · 2017-01-13T01:52:54Z

Those diagnostics about memoryless etc. have been seen elsewhere (rdar://29715586, running swift build using an internal Xcode build a month ago). I doubt that they are your fault, but I don't know if the cause has been found yet.

jrose-apple · 2017-01-13T02:25:50Z

Typo-correction is what brings them up at all, Sema not permitting use of deprecated/unavailable constructs in deprecated/unavailable contexts is why they get diagnosed. Just hasn't been a priority.

modocache · 2017-01-13T02:32:30Z

Should I change the REPL tests in order to prevent the failures from occurring?

I'm curious why my changes seem to have angered the beast. I'm double-checking at the moment, but I'm pretty sure these failures occur consistently on my branch, but not on master.

jrose-apple · 2017-01-13T03:21:18Z

I don't immediately see what your changes would have done. It seems like they'd only bring in more declarations (macros), not remove any existing ones.

modocache · 2017-01-13T14:16:55Z

@swift-ci please smoke test

modocache · 2017-01-19T03:24:49Z

Ping @jrose-apple -- what would you recommend I do here: change the test so that it doesn't happen to fail? Or dig in deeper to discover why my changes are causing the failure?

If the latter, could you please share any relevant details from rdar://29715586? It's not on OpenRadar.

jrose-apple · 2017-01-19T16:40:44Z

I'm still surprised your changes have anything to do with the failing tests (which unfortunately have been wiped from the public bot), so yes, I think we need to figure out what's going on.

The Radar just says that importing certain types in Metal results in us making variables like static var unavailableAlias { return unavailableOrig } where both unavailableAlias and unavailableOrig are marked unavailable. But that shouldn't happen more often in your code than before unless lookups are triggering typo-correction, which I wouldn't expect them to.

jrose-apple · 2017-02-02T18:24:36Z

Well, I fixed the issue with unavailable aliases, so we won't see that particular problem anymore. I guess we can start over from there.

Resolves SR-2402. Import function-like macros that take no arguments, assuming their bodies are something ClangImporter would otherwise be able to import. * Modify `shouldIgnoreMacro` such that it only ignores function-like macros if they take more than zero arguments. * ClangImporter's macro importing normally creates `VarDecl`. Modify that to create FuncDecl if the original macro is function-like.

modocache · 2017-02-02T18:51:22Z

@swift-ci please test

modocache · 2017-02-02T18:51:42Z

Sorry for letting this languish!

swift-ci · 2017-02-02T18:51:45Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 4d4e6d87883513608ba36e2155eeb8b29789ba34
Test requested by - @modocache

swift-ci · 2017-02-02T18:51:49Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 4d4e6d87883513608ba36e2155eeb8b29789ba34
Test requested by - @modocache

modocache · 2017-02-02T18:52:36Z

Oh, maybe I angered @swift-ci by requesting a test after force pushing to my branch. I'll try again.

modocache · 2017-02-02T18:52:46Z

@swift-ci please smoke test

jrose-apple · 2017-02-02T19:24:27Z

I let it languish just as much as you did. Glad to pick it up again now.

modocache · 2017-02-02T21:17:57Z

Hmm, looks like the same problem test failed, Interpreter/SDK/missing_imports_repl.swift.

I probably won't have a ton of time to work on this, at least for another week or so. Please feel free to grab this task from me if you'd like to make progress on it before then!

modocache · 2017-03-08T19:12:34Z

I don't have a ton of time to work on this anymore. Still, it was a fun way to learn more about ClangImporter, so thank you!

modocache requested review from milseman and jrose-apple January 3, 2017 03:15

CodaFi reviewed Jan 3, 2017

View reviewed changes

slavapestov assigned jrose-apple Jan 10, 2017

modocache force-pushed the sr-2402-import-nullary-function-macros branch from c850057 to 8761118 Compare January 12, 2017 17:08

modocache force-pushed the sr-2402-import-nullary-function-macros branch 2 times, most recently from 6a4b455 to 7c1f4d1 Compare January 12, 2017 20:20

jrose-apple suggested changes Jan 12, 2017

View reviewed changes

modocache force-pushed the sr-2402-import-nullary-function-macros branch from 7c1f4d1 to e3eeb56 Compare January 12, 2017 23:32

jrose-apple approved these changes Jan 13, 2017

View reviewed changes

modocache force-pushed the sr-2402-import-nullary-function-macros branch from e3eeb56 to 4d4e6d8 Compare January 13, 2017 01:53

modocache force-pushed the sr-2402-import-nullary-function-macros branch from 4d4e6d8 to 8f7baa3 Compare February 2, 2017 18:50

modocache closed this Mar 8, 2017

modocache mannequin mentioned this pull request Apr 25, 2022

[SR-2402] Clang importer should import nullary function-like macros #45009

Open

		@@ -0,0 +1,13 @@
		// Other ClangImporter tests verify that function-like C macros are imported
		// into Swift so that identifiers are resolved correclty. This test verifies

[SR-2402][ClangImporter] Import nullary function macros #6530

[SR-2402][ClangImporter] Import nullary function macros #6530

Uh oh!

Conversation

modocache commented Jan 3, 2017

Uh oh!

CodaFi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

modocache commented Jan 3, 2017

Uh oh!

jrose-apple commented Jan 4, 2017

Uh oh!

modocache commented Jan 4, 2017

Uh oh!

jrose-apple commented Jan 4, 2017

Uh oh!

modocache commented Jan 4, 2017

Uh oh!

slavapestov commented Jan 4, 2017

Uh oh!

jrose-apple commented Jan 4, 2017

Uh oh!

slavapestov commented Jan 9, 2017

Uh oh!

slavapestov commented Jan 9, 2017

Uh oh!

swift-ci commented Jan 9, 2017

Uh oh!

modocache commented Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple commented Jan 9, 2017

Uh oh!

slavapestov commented Jan 11, 2017

Uh oh!

modocache commented Jan 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

modocache commented Jan 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple commented Jan 12, 2017

Uh oh!

modocache commented Jan 12, 2017

Uh oh!

modocache commented Jan 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

modocache Jan 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

modocache commented Jan 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

modocache commented Jan 13, 2017

Uh oh!

gparker42 commented Jan 13, 2017

Uh oh!

jrose-apple commented Jan 13, 2017

Uh oh!

modocache commented Jan 13, 2017

CodaFi left a comment •

edited

Loading

modocache commented Jan 9, 2017 •

edited

Loading

modocache commented Jan 12, 2017 •

edited

Loading

modocache commented Jan 12, 2017 •

edited

Loading

modocache Jan 12, 2017 •

edited

Loading