Resolve and init modules mentioned on command line #18724

mppf · 2021-11-15T14:52:15Z

Related to PR #13369

This PR

resolves and arranges to run module init/deinit for any top-level module mentioned on the command line
changes to setting FLAG_MODULE_FROM_COMMAND_LINE_FILE only for top-level modules mentioned on the command line (not for nested modules). To keep main-module inference working I needed to add a helper function isModuleOrSubmoduleFromCommandLine in ModuleSymbol.cpp.
removes currentFileNamedOnCommandLine and instead takes the approach of setting on top-level modules parsed from command line files after parsing.
updates a comment about how to add a flag since the flags are now in the dyno library

Reviewed by @lydia-duncan - thanks!

full local testing

bradcray · 2021-11-15T19:11:42Z

I'm seeing 184 errors on this branch, most of which exhibit as internal errors of one form or another with the LLVM back-end. E.g., for test/classes/initializers/secondary/inDiffModule.chpl, the LLVM back-end gets an access error (internal error: COD-CG--BOL-2765 chpl version 1.26.0 pre-release (88ac3197ab) ) at the following stack trace:

  * frame #0: 0x000000010016b037 chpl`Symbol::isRef(this=0x0000000111b840f0) at symbol.cpp:236:21
    frame #1: 0x0000000100073e90 chpl`pruneVisit(sym=0x0000000111b840f0, fns=0x00007ffeefbff2f0, types=0x00007ffeefbff2c0) at astutil.cpp:858:14
    frame #2: 0x00000001000741c7 chpl`pruneVisitFn(fn=0x0000000113e23ed0, fns=0x00007ffeefbff2f0, types=0x00007ffeefbff2c0) at astutil.cpp:891:7
    frame #3: 0x0000000100073e69 chpl`pruneVisit(sym=0x0000000113e23ed0, fns=0x00007ffeefbff2f0, types=0x00007ffeefbff2c0) at astutil.cpp:855:5
    frame #4: 0x000000010006cc5e chpl`visitVisibleFunctions(fns=0x00007ffeefbff2f0, types=0x00007ffeefbff2c0) at astutil.cpp:906:5
    frame #5: 0x000000010006ca3f chpl`prune() at astutil.cpp:1105:3
    frame #6: 0x000000010006d469 chpl`prune2() at astutil.cpp:1122:17
    frame #7: 0x000000010036e840 chpl`runPass(tracker=0x00007ffeefbff3e0, passIndex=33, isChpldoc=false) at runpasses.cpp:208:3
    frame #8: 0x000000010036e6bd chpl`runPasses(tracker=0x00007ffeefbff3e0, isChpldoc=false) at runpasses.cpp:170:5
    frame #9: 0x0000000100360987 chpl`main(argc=5, argv=0x00007ffeefbff458) at driver.cpp:1756:3

The C back-end seems to complain about chpl__init_B() not having a prototype in the generated code (and is one of the cases I feel like I've been seeing lately where the compiler doesn't generate a non-zero exit code even though the C compilation broke, so I'm looking into that next). Given the nature of the test, it makes me wonder whether something more needs to be done to handle the case of two modules defined within a single file on the command-line.

[edit: Sometimes I'm seeing setfaults / bus errors from the C back-end as well, so I suspect there's a valgrind/asan-style failure at the heart of this]

These failures were fishy enough, and the others similar enough on the surface, that I didn't look at other failure cases much.

mppf · 2021-11-15T19:29:25Z

Thanks, I'll have a look and see if I can fix inDiffModule.chpl. I can see it has --verify errors currently.

mppf · 2021-11-15T19:39:23Z

I have a fix for inDiffModule and I'm going to go ahead and launch a test run.

mppf · 2021-11-15T21:06:15Z

Here is the test result with --verify

[Error matching compiler output for constrained-generics/basic/set1/error-cg-arg]
[Error matching program output for extern/bradc/require/useModAsString]
[Error matching compiler output for extern/require/requireInSubMod]
[Error matching compiler output for extern/require/requireInTopMod]
[Error matching program output for modules/diten/main-module-flag/multiple_modules_no_main2]
[Error matching program output for modules/diten/main-module-flag/multiple_modules_no_main3]
[Error matching program output for modules/errors/code-outside-of-module-error]
[Error matching program output for modules/noakes/mod-init-fails]
[Error matching compiler output for modules/sungeun/resolution/module-resolution-issue (compopts: 1)]
[Error matching program output for modules/sungeun/resolution/module-resolution-issue (compopts: 2)]
[Error matching compiler output for modules/sungeun/no-use-class]
[Error matching compiler output for modules/sungeun/no-use-enum]
[Error matching compiler output for modules/sungeun/no-use-record]
[Error matching compiler output for modules/sungeun/no-use-type-alias]
[Error matching program output for optimizations/deadCodeElimination/elliot/deadModuleElim]
[Error matching program output for performance/vectorization/vectorPragmas/loopsInForallNoVector]
[Error matching program output for performance/vectorization/vectorPragmas/zipIterNoVector]
[Error matching compiler output for release/examples/primers/errorHandling]
[Error matching program output for release/examples/primers/modules]
[Summary: #Successes = 13261 | #Failures = 19 | #Futures = 0]

bradcray · 2021-11-15T21:12:11Z

error-cg-arg raises an interesting question about whether nested modules in files specified on the command-line should also be initialized even if they aren't used. I don't have an immediate intuition about that question, but if I had to pick a side quickly, I'd probably say "no."

bradcray · 2021-11-15T21:29:42Z

With a quick run-through of the rest of the tests, it looked like 1/2 - 1/3 were other cases of nested modules. The noakes, sungeun, elliot, and primers looked like they'd require another look.

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

I figured out I could remove the parser global currentFileNamedOnCommandLine as well. --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

mppf · 2022-02-14T16:07:41Z

I've adjusted it to consider only top-level modules for FLAG_MODULE_FROM_COMMAND_LINE_FILE and these are some failures to look at next:

[Error matching compiler output for extern/require/requireInTopMod]
[Error matching compiler output for modules/noakes/mod-init-fails]
[Error matching compiler output for modules/sungeun/no-use-class]
[Error matching compiler output for modules/sungeun/no-use-enum]
[Error matching compiler output for modules/sungeun/no-use-record]
[Error matching compiler output for modules/sungeun/no-use-type-alias]
[Error matching compiler output for modules/sungeun/resolution/module-resolution-issue (compopts: 1)]
[Error matching program output for modules/sungeun/resolution/module-resolution-issue (compopts: 2)]
[Error matching program output for optimizations/deadCodeElimination/elliot/deadModuleElim]
[Error matching program output for performance/vectorization/vectorPragmas/loopsInForallNoVector]
[Error matching program output for performance/vectorization/vectorPragmas/zipIterNoVector]

bradcray · 2022-02-16T05:16:38Z

Taking a look, it seems that most of these failures are "expected" due to the change, with the exception of a pair of internal errors and two tests that I didn't yet sort out what was happening. Bottom line, this looks close to me:

[Error matching compiler output for extern/require/requireInTopMod]

This was checking that a non-used module didn't affect requirements, but now it will, as it arguably should.

[Error matching compiler output for modules/sungeun/no-use-class]
[Error matching compiler output for modules/sungeun/no-use-enum]
[Error matching compiler output for modules/sungeun/no-use-record]
[Error matching compiler output for modules/sungeun/no-use-type-alias]
[Error matching program output for modules/sungeun/resolution/module-resolution-issue (compopts: 2)]

These all seem to be checking that an unused module doesn't get compiled, but not it will be, so new errors are generated.

[Error matching program output for optimizations/deadCodeElimination/elliot/deadModuleElim]

New dead modules are eliminated because they were considered more than usual?

[Error matching compiler output for modules/noakes/mod-init-fails]
[Error matching compiler output for modules/sungeun/resolution/module-resolution-issue (compopts: 1)]

Internal error—needs a look

[Error matching program output for performance/vectorization/vectorPragmas/loopsInForallNoVector]
[Error matching program output for performance/vectorization/vectorPragmas/zipIterNoVector]

Wasn't obvious to me what was happening here.

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Modules from files named on the command line are now treated differently, so update this test to 'use' / 'import' to make sure to parse the other modules. That allows it to continue to work as it did. --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

These tests were relying on the module InvokeIters which is mentioned on the command line not being resolved. But now it is, so adjust these tests accordingly. --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

I don't understand why, but this function is currently implemented to only check modules named on the command line for the 'main' function. See issue chapel-lang#19258. --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

lydia-duncan

Just a couple of suggestions but nothing major

compiler/AST/ModuleSymbol.cpp

compiler/include/flags.h

compiler/optimizations/deadCodeElimination.cpp

test/extern/require/requireInTopMod.chpl

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

@bradcray

Adjust spec wording for main module details Resolves #19258. To clarify the docs for issue #19258. * Describes identifying the "main module" in a new section * use "procedure" instead of "function" when referring to a `proc` (including for `main`) in the Modules chapter * improved the module initialization section to talk about what happens when a particular module is initialized instead of generally describing all module initialization at once * improved the module initialization to indicate top-level modules in files mentioned on the command line are initialized (from PR #18724) Reviewed by @bradcray - thanks!

@aconsroe-hpe

Restore intended PTRANS.valgrind behavior [reviewed by @aconsroe-hpe] This test's behavior started changing when we started running initialization for all modules named on the command-line in #18724. This is because we were calling main once for the `--main-module` of PTRANS and once for the explicit call to main() in the PTRANS.valgrind module. When writing this, the author (could've been me) presumably imagined that their explicit call to main() was happening and necessary when in fact neither was true. Since we permit users to call main(), the test started working differently / as now intended when #18724 was merged, but not how we expected. Here, I'm removing the explicit call to main() to restore the expected behavior. Thanks to @aconsroe-hpe for helping to diagnose this!

--- Signed-off-by: Paul Cassella <gitgitgit@manetheren.bigw.org>

@vasslitvinov

Remove dead test files and update findUnusedTestFiles Remove a few .bad files whose .futures were removed in #17353 and #18724, and a .compopts file that appears never to have been used. The affected directories pass testing. Update findUnusedTestFiles to skip all .notest files in test/mason due to how often these are to skip directories created at runtime. Also handle a few more special cases in test/distributions/robust/arithmetic/. reviewed by @vasslitvinov and @e-kayrakli merged by @vasslitvinov on behalf of @cassella

This PR changes from the current approach of generating a `ServerRegistration.chpl` file that calls the `registerMe()` function required in all modules to instead move the contents of the `registerMe()` to module-level scope that will add the functions to the command map upon module initialization. All modules that are to be modularly included in the server are now directly specified on the command line. This change is enabled by the work from chapel-lang/chapel#18724, which initializes all modules specified on the command line of a `chpl` call, even if the modules aren't explicitly `use`d. This means that Arkouda will require at least Chapel 1.26 after this change.

…ommand line (#1606) * Update modular build process to initialize modules on command line This PR changes from the current approach of generating a `ServerRegistration.chpl` file that calls the `registerMe()` function required in all modules to instead move the contents of the `registerMe()` to module-level scope that will add the functions to the command map upon module initialization. All modules that are to be modularly included in the server are now directly specified on the command line. This change is enabled by the work from chapel-lang/chapel#18724, which initializes all modules specified on the command line of a `chpl` call, even if the modules aren't explicitly `use`d. This means that Arkouda will require at least Chapel 1.26 after this change. * Convert rest of modules * Update python script to handle non-src dir files * Remove 1.25.1 compat check * Add space caught by Pierce * Update modular building docs * Update MODULAR.md fixed a typo `pat` -> `path` Co-authored-by: pierce314159 <48131946+pierce314159@users.noreply.github.com>

…dules on command line (Bears-R-Us#1606) * Update modular build process to initialize modules on command line This PR changes from the current approach of generating a `ServerRegistration.chpl` file that calls the `registerMe()` function required in all modules to instead move the contents of the `registerMe()` to module-level scope that will add the functions to the command map upon module initialization. All modules that are to be modularly included in the server are now directly specified on the command line. This change is enabled by the work from chapel-lang/chapel#18724, which initializes all modules specified on the command line of a `chpl` call, even if the modules aren't explicitly `use`d. This means that Arkouda will require at least Chapel 1.26 after this change. * Convert rest of modules * Update python script to handle non-src dir files * Remove 1.25.1 compat check * Add space caught by Pierce * Update modular building docs * Update MODULAR.md fixed a typo `pat` -> `path` Co-authored-by: pierce314159 <48131946+pierce314159@users.noreply.github.com>

mppf mentioned this pull request Nov 15, 2021

Should adding a module to the compilation command initialize the module? #18714

Closed

mppf added the post-release label Nov 29, 2021

mppf added 4 commits February 14, 2022 10:41

Resolve and init modules mentioned on command line

7587143

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Don't dead-code-eliminate modules named on command line

427840d

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Update a few tests where output change makes sense

4212c01

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

MODULE_FROM_COMMAND_LINE_FILE only for top-level

3a1a342

I figured out I could remove the parser global currentFileNamedOnCommandLine as well. --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

mppf force-pushed the issue-18714 branch from f9d38c9 to 3a1a342 Compare February 14, 2022 16:06

mppf added 9 commits February 16, 2022 09:56

Adjust requireInTopMod.chpl for new behavior

e488275

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Adjust tests for new behavior

30e8ac5

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Fix problem in removeUnusedModules

2b7449a

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Update .good file for new behavior

da2eb44

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Update test output for new behavior

863f729

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Fix loopsInForallNoVector & zipIterNoVector

039b589

These tests were relying on the module InvokeIters which is mentioned on the command line not being resolved. But now it is, so adjust these tests accordingly. --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Add test from issue chapel-lang#18714 including deinits

a59982d

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

mppf force-pushed the issue-18714 branch from d9c8ae3 to 09fa69f Compare February 16, 2022 18:41

Fix useModAsString2/3 .good based on changes in test

0729a61

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

mppf marked this pull request as ready for review February 16, 2022 19:51

mppf removed the post-release label Feb 16, 2022

lydia-duncan approved these changes Feb 16, 2022

View reviewed changes

compiler/AST/ModuleSymbol.cpp Outdated Show resolved Hide resolved

compiler/include/flags.h Show resolved Hide resolved

compiler/optimizations/deadCodeElimination.cpp Show resolved Hide resolved

test/extern/require/requireInTopMod.chpl Show resolved Hide resolved

Rename new function per review suggestion

7a81307

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

mppf mentioned this pull request Feb 17, 2022

Adjust spec wording for main module details #19259

Merged

Un-future two future and update their .good files

f91937d

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

mppf merged commit 195c457 into chapel-lang:main Feb 17, 2022

mppf deleted the issue-18714 branch February 17, 2022 19:52

bradcray mentioned this pull request Feb 22, 2022

Restore intended PTRANS.valgrind behavior #19289

Merged

cassella added a commit to cassella/chapel that referenced this pull request Mar 27, 2022

Remove two .bad files whose .futures were removed in chapel-lang#18724

574dd8c

--- Signed-off-by: Paul Cassella <gitgitgit@manetheren.bigw.org>

cassella mentioned this pull request Mar 27, 2022

Remove dead test files and update findUnusedTestFiles #19552

Merged

bmcdonald3 mentioned this pull request Jul 11, 2022

Proposal to update modular build once 1.26 is required by Arkouda Bears-R-Us/arkouda#1572

Closed

bmcdonald3 mentioned this pull request Jul 19, 2022

Closes #1572: Update modular build process to initialize modules on command line Bears-R-Us/arkouda#1606

Merged

lydia-duncan mentioned this pull request May 24, 2024

How should 'require something.chpl' interact with finding the main module? #25112

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve and init modules mentioned on command line #18724

Resolve and init modules mentioned on command line #18724

mppf commented Nov 15, 2021 •

edited

Loading

bradcray commented Nov 15, 2021 •

edited

Loading

mppf commented Nov 15, 2021

mppf commented Nov 15, 2021

mppf commented Nov 15, 2021

bradcray commented Nov 15, 2021

bradcray commented Nov 15, 2021

mppf commented Feb 14, 2022

bradcray commented Feb 16, 2022

lydia-duncan left a comment

Resolve and init modules mentioned on command line #18724

Resolve and init modules mentioned on command line #18724

Conversation

mppf commented Nov 15, 2021 • edited Loading

bradcray commented Nov 15, 2021 • edited Loading

mppf commented Nov 15, 2021

mppf commented Nov 15, 2021

mppf commented Nov 15, 2021

bradcray commented Nov 15, 2021

bradcray commented Nov 15, 2021

mppf commented Feb 14, 2022

bradcray commented Feb 16, 2022

lydia-duncan left a comment

Choose a reason for hiding this comment

mppf commented Nov 15, 2021 •

edited

Loading

bradcray commented Nov 15, 2021 •

edited

Loading