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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

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



================
Comment at: lib/Exchange/JSONExporter.cpp:287
 
-  for (unsigned i = 0; i < isl_set_dim(OldContext, isl_dim_param); i++) {
+  // Check if the context parsed is valid.
+  if (!NewContext) {
----------------
Suggestion: "Check whether the context was parsed successfully."


================
Comment at: lib/Exchange/JSONExporter.cpp:335
+  Json::Value statements = JScop["statements"];
+  // Check wether the number of indices equals the number of statements
+  if (statements.size() != S.getSize()) {
----------------
w**h**ether


================
Comment at: lib/Exchange/JSONExporter.cpp:338
+    errs() << "The number of indices and the number of statements are not "
+           << "equals.\n";
+    return false;
----------------
not equal~~s~~

or

differ


================
Comment at: lib/Exchange/JSONExporter.cpp:605-608
+  // Check if key 'arrays' is present.
   Json::Value Arrays = JScop["arrays"];
-
-  if (Arrays.size() == 0)
+  if (Arrays.size() == 0) {
+    errs() << "JScop file has no key named 'arrays'.\n";
----------------
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.


================
Comment at: lib/Exchange/JSONExporter.cpp:626
+    if (!Arrays[ArrayIdx].isMember("type")) {
+      errs() << "Array at index " << ArrayIdx << " has not key 'type'.\n";
+      return false;
----------------
has no~~t~~ key


================
Comment at: lib/Exchange/JSONExporter.cpp:633
+    if (!ElementType) {
+      errs() << "Array parsed is not valid for Index " << ArrayIdx << ".\n";
       return false;
----------------
`index` (lower case i)


================
Comment at: test/JSONExporter/rlr2.ll:1
+; RUN: (2>&1 opt %loadPolly -polly-import-jscop-dir=%S -polly-import-jscop -polly-ast -polly-ast-detect-parallel -analyze < %s) | FileCheck %s 
+; RUN: (2>&1 opt %loadPolly -polyhedral-info -polly-check-parallel -analyze < %s) | FileCheck %s -check-prefix=PINFO 
----------------
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.


https://reviews.llvm.org/D32739