[Polly] JSONImporter misses checks whether the data it imports makes sense.

[niosega]

Summary

Without this patch, the JSONImporter did not verify if the data it loads were correct or not (Bug 32543). I add some checks in the JSONImporter class and some test cases.

Here are the checks (and test cases) I added :

JSONImporter::importContext

• The "context" key does not exist.
• The context was not parsed successfully by ISL.
• The isl_set has the wrong number of parameters.
• The isl_set is not a parameter set.

JSONImporter::importSchedule

• The "statements" key does not exist.
• There is not the right number of statement in the file.
• The "schedule" key does not exist.
• The schedule was not parsed successfully by ISL.

JSONImporter::importAccesses

• The "statements" key does not exist.
• There is not the right number of statement in the file.
• The "accesses" key does not exist.
• There is not the right number of memory accesses in the file.
• The "relation" key does not exist.
• The memory access was not parsed successfully by ISL.

JSONImporter::areArraysEqual

• The "type" key does not exist.
• The "sizes" key does not exist.
• The "name" key does not exist.

JSONImporter::importArrays
/!\ Do not check if there is an key name "arrays" because it is not considered as an error.
All checks are already in place or implemented in JSONImporter::areArraysEqual

[niosega]

The first one is not complete.

[Meinersbur]

Please don't copy-paste my suggestions into the summary, especially not because they do not reflect what you actually implemented.

lib/Exchange/JSONExporter.cpp

287

Suggestion: "Check whether the context was parsed successfully."

335

whether

338

not equals

or

differ

605–608

A SCoP doesn't strictly need arrays.

So far none of your tests has an arrays key, meaning all of them would fail if there is no error before.

626

has not key

633

index (lower case i)

test/JSONExporter/rlr2.ll

1 ↗

(On Diff #97431)

Window's implementation of llvm-lit doesn't handle this:

********************
UNRESOLVED: Polly :: JSONExporter/rlr.ll (397 of 869)
******************** TEST 'Polly :: JSONExporter/rlr.ll' FAILED ********************
Exception during script execution:
Traceback (most recent call last):
  File "C:\Users\Meinersbur\src\llvm\utils\lit\lit\run.py", line 187, in execute_test
    result = test.config.test_format.execute(test, lit_config)
  File "C:\Users\Meinersbur\src\llvm\utils\lit\lit\formats\shtest.py", line 12, in execute
    self.execute_external)
  File "C:\Users\Meinersbur\src\llvm\utils\lit\lit\TestRunner.py", line 1082, in executeShTest
    res = _runShTest(test, litConfig, useExternalSh, script, tmpBase)
  File "C:\Users\Meinersbur\src\llvm\utils\lit\lit\TestRunner.py", line 1030, in _runShTest
    res = executeScriptInternal(test, litConfig, tmpBase, script, execdir)
  File "C:\Users\Meinersbur\src\llvm\utils\lit\lit\TestRunner.py", line 548, in executeScriptInternal
    exitCode, timeoutInfo = executeShCmd(cmd, shenv, results, timeout=litConfig.maxIndividualTestTime)
  File "C:\Users\Meinersbur\src\llvm\utils\lit\lit\TestRunner.py", line 137, in executeShCmd
    finalExitCode = _executeShCmd(cmd, shenv, results, timeoutHelper)
  File "C:\Users\Meinersbur\src\llvm\utils\lit\lit\TestRunner.py", line 245, in _executeShCmd
    res = _executeShCmd(cmd.lhs, shenv, results, timeoutHelper)
  File "C:\Users\Meinersbur\src\llvm\utils\lit\lit\TestRunner.py", line 353, in _executeShCmd
    r[2] = open(redir_filename, r[1])
FileNotFoundError: [Errno 2] No such file or directory: 'C:\\Users\\Meinersbur\\src\\llvm\\tools\\polly\\test\\JSONExporter\\rlr.ll)'

Note that the RUN line is not really a shell command, so () doesn't launch a sub-shell. llvm-lit has its own parser for them.

Try to avoid 2>&1 if possible. How two output streams are merged is undefined. Now I think there is no direct way to only to only FileCheck stderr, but you can disable stderr with --disable-output. Or you can use 2>&1 >/dev/null which is explicitly supported under Windows.

[Meinersbur]

Thank you for the patch, it looks very promising.

When uploading patches, please use also add context (with git: git diff -U999999, see: http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface)

test/CMakeLists.txt

9 ↗

(On Diff #97431)

Mmmh, that list looks very outdated. It is also only used to make polly-test-${dir} targets and only in out-of-LLVM-source builds. I think we should remove it (I'll do this in a separate commit)

test/JSONExporter/rlr2.ll

2 ↗

(On Diff #97431)

I don't think the -analyze output is interesting for these test cases.

[simbuerg]

lib/Exchange/JSONExporter.cpp

382

'whether'

640

has no key

649

has no key

[niosega]

This diff takes into account the remarks of Michael Kruse and Andreas Simbuerger.

[Meinersbur]

Are you going to implement checks for importAccesses as well?

[grosser]

@niosega : Any updates here?

[niosega]

This comment was removed by niosega.

lib/Exchange/JSONExporter.cpp

605–608

There was already an if to check whether the Json contains a arrays key or not. I have just added an output to inform the user. Should I remove my output ?

[niosega]

I am currently working on adding checks for importAccess and cleaning the test cases (explain what they are doing, better naming .. )

[Meinersbur]

lib/Exchange/JSONExporter.cpp

605–608

Since it returns true, it looks like normal operation. Error output would be confusing. So, set, you can remove it.

[Meinersbur]

For some reason, I cannot apply the patch. The error I get:

Checking patch dev/null => test/JSONExporter/ImportAccesses/ImportAccesses-Bad-relation.ll...
error: dev/null: does not exist in index

The patch is also not based on the latest trunk anyway (CMakeLists.txt has changed). Please rebase to current trunk. Also, please add more context. as explained here; http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface . Please also update the summary.

Thank you for the update. Except for that I cannot apply the patch and try out, this looks very good.

lib/Exchange/JSONExporter.cpp

475

Please avoid changing unrelated parts. It's just noise in the diff.

520

Please keep the empty line, if there was one originally. Most of the time, we have an empty line before before comment lines.

624

Please add a newline

688–691

There is no effective change here. Please keep the original version.

test/JSONExporter/ImportSchedule/ImportSchedule-Wrong-number-statements.ll

5 ↗

(On Diff #99256)

Redundant ;

[niosega]

Take into account the remarks of Tobias concerning the line before comments.
Also rebase with the last version of master.

[Meinersbur]

Thank you. Especially for the many test cases and otherwise high quality.

I am going to commit it on your behalf in a moment.

lib/Exchange/JSONExporter.cpp

 
 bool JSONImporter::importContext(Scop &S, Json::Value &JScop) {
   isl_set *OldContext = S.getContext();
+
+  if (!JScop.isMember("context")) {
+    errs() << "JScop file has no key named 'context'.\n";
+    isl_set_free(OldContext);
+    return false;
+  }
+
   isl_set *NewContext =
       isl_set_read_from_str(S.getIslCtx(), JScop["context"].asCString());
 
+  if (!isl_set_is_params(NewContext)) {
+    errs() << "The isl_set is not a parameter set.\n";
+    isl_set_free(NewContext);
+    isl_set_free(OldContext);
+    return false;
+  }
+
   for (unsigned i = 0; i < isl_set_dim(OldContext, isl_dim_param); i++) {
     isl_id *Id = isl_set_get_dim_id(OldContext, isl_dim_param, i);
     NewContext = isl_set_set_dim_id(NewContext, isl_dim_param, i, Id);