Page MenuHomePhabricator

[OpenMP] Fix false error report of array subscription for templated indexes.
AbandonedPublic

Authored by h-joo on Apr 22 2020, 8:07 AM.

Details

Reviewers
ABataev
jdoerfert
Summary

This is a fix for Bug 45383.

This revision fixes the error reported for array subscription for the cases where the array index is a template parameter.
Note that the ArraySubscriptExpr::getBase() might return something which might not be a base in the cases where either LHS or RHS of the ArraySubscriptExpr is a template declaration - I think the fundemental fix would include more changes with ArraySubscriptExpr::getBase() in the presence of template dependent types, but I thought it would involve a bigger scope of refactoring since many of the code is touching it.

Please also keep in mind this is my first patch, so I would be very glad for any kind of comment if I did something wrong.

Diff Detail

Unit TestsFailed

TimeTest
910 mslibc++.libcxx/utilities/function_objects::Unknown Unit Message ("")
Script: -- : 'RUN: at line 14'; /usr/bin/clang++ /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/libcxx/utilities/function.objects/func.blocks.sh.cpp -v -Werror=thread-safety -std=c++2a -include /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/support/nasty_macros.h -nostdinc++ -I/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/include -I/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/support -ftemplate-depth=270 -Wall -Wextra -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Wuser-defined-warnings -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -L/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./lib -Wl,-rpath,/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./lib -nodefaultlibs -lc++experimental -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -o /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/projects/libcxx/test/libcxx/utilities/function.objects/Output/func.blocks.sh.cpp.tmp.exe -fblocks
820 mslibc++.libcxx/utilities/optional::Unknown Unit Message ("")
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/libcxx/utilities/optional/block.objc.pass.mm -Werror -v -Werror=thread-safety -std=c++2a -include /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/support/nasty_macros.h -nostdinc++ -I/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/include -I/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/support -ftemplate-depth=270 -Wall -Wextra -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Wuser-defined-warnings -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -fblocks -L/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./lib -Wl,-rpath,/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./lib -nodefaultlibs -lc++experimental -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -o /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/projects/libcxx/test/libcxx/utilities/optional/Output/block.objc.pass.mm.tmp.exe
11,570 mslldb-api.commands/expression/import-std-module/basic::Unknown Unit Message ("")
Script: -- /usr/bin/python /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/lldb/test/API/dotest.py --arch x86_64 -s /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lldb-test-traces -S nm -u CXXFLAGS -u CFLAGS --env ARCHIVER=/usr/bin/ar --env OBJCOPY=/usr/bin/objcopy --env LLVM_LIBS_DIR=/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./lib --build-dir /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lldb-test-build.noindex --lldb-module-cache-dir /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lldb-test-build.noindex/module-cache-lldb/lldb-api --clang-module-cache-dir /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lldb-test-build.noindex/module-cache-clang/lldb-api --executable /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./bin/lldb --compiler /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./bin/clang --dsymutil /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./bin/dsymutil --filecheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./bin/FileCheck --lldb-libs-dir /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./lib /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/lldb/test/API/commands/expression/import-std-module/basic -p TestImportStdModule.py
3,510 mslldb-api.commands/expression/import-std-module/conflicts::Unknown Unit Message ("")
Script: -- /usr/bin/python /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/lldb/test/API/dotest.py --arch x86_64 -s /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lldb-test-traces -S nm -u CXXFLAGS -u CFLAGS --env ARCHIVER=/usr/bin/ar --env OBJCOPY=/usr/bin/objcopy --env LLVM_LIBS_DIR=/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./lib --build-dir /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lldb-test-build.noindex --lldb-module-cache-dir /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lldb-test-build.noindex/module-cache-lldb/lldb-api --clang-module-cache-dir /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lldb-test-build.noindex/module-cache-clang/lldb-api --executable /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./bin/lldb --compiler /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./bin/clang --dsymutil /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./bin/dsymutil --filecheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./bin/FileCheck --lldb-libs-dir /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./lib /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/lldb/test/API/commands/expression/import-std-module/conflicts -p TestStdModuleWithConflicts.py
5,870 mslldb-api.commands/expression/import-std-module/deque-basic::Unknown Unit Message ("")
Script: -- /usr/bin/python /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/lldb/test/API/dotest.py --arch x86_64 -s /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lldb-test-traces -S nm -u CXXFLAGS -u CFLAGS --env ARCHIVER=/usr/bin/ar --env OBJCOPY=/usr/bin/objcopy --env LLVM_LIBS_DIR=/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./lib --build-dir /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lldb-test-build.noindex --lldb-module-cache-dir /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lldb-test-build.noindex/module-cache-lldb/lldb-api --clang-module-cache-dir /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lldb-test-build.noindex/module-cache-clang/lldb-api --executable /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./bin/lldb --compiler /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./bin/clang --dsymutil /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./bin/dsymutil --filecheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./bin/FileCheck --lldb-libs-dir /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/./lib /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/lldb/test/API/commands/expression/import-std-module/deque-basic -p TestDequeFromStdModule.py
View Full Test Results (43 Failed)

Event Timeline

h-joo created this revision.Apr 22 2020, 8:07 AM

Can we create a test case that shows even if it is a dependent type we will eventuall issue an error if it is not an addressable lvalue or array item?
If this is the part that needs refactoring to work, we should add the test with a TODO.

h-joo updated this revision to Diff 259326.EditedApr 22 2020, 10:09 AM

Can we create a test case that shows even if it is a dependent type we will eventuall issue an error if it is not an addressable lvalue or array item?
If this is the part that needs refactoring to work, we should add the test with a TODO.

Added a test to check the test triggers in presence of lvalue expression. Although the test does not trigger line 15912, but rather line 15905. I tried some examples and it seems like after during the template instantiation, while an ArraySubscriptExpr is being constructed, it already checks whether it's a pointer type or an array type, thus, I am thinking this check in line 15912 might actually be redundant?

This looks reasonable to me. @ABataev WDYT?

This looks reasonable to me. @ABataev WDYT?

I would add a positive test with -ast-print

clang/lib/Sema/SemaOpenMP.cpp
15913

Just QualType BaseType = ASE->getBase()->getType().getNonReferenceType(); and drop the call for getNonReferenceType() in later checks.

h-joo updated this revision to Diff 259343.EditedApr 22 2020, 11:30 AM

This looks reasonable to me. @ABataev WDYT?

I would add a positive test with -ast-print

  1. Changed into a positive test with -ast-print
  2. Just QualType BaseType = ASE->getBase()->getType().getNonReferenceType(); and dropped all the call for getNonReferenceType() in later checks.

Thank you for your time for the review! I do have one more question to ask. I don't understand the log of the build failure, may I ask for some help to understand the failures?

h-joo marked an inline comment as done.EditedApr 30 2020, 2:50 AM

Sorry, I feel like I'm holding back the fix for this bug since I'm stuck on how to proceed with the build failure, @ABataev would you like to proceed with the other revision that you were working on? If you would, I will drop this one then.

Sorry, I feel like I'm holding back the fix for this bug since I'm stuck on how to proceed with the build failure, @ABataev would you like to proceed with the other revision that you were working on? If you would, I will drop this one then.

Don't worry. What build failure exactly? The lldb libc++ failures shown here in phab or something else?

h-joo added a comment.Apr 30 2020, 6:10 AM

Sorry, I feel like I'm holding back the fix for this bug since I'm stuck on how to proceed with the build failure, @ABataev would you like to proceed with the other revision that you were working on? If you would, I will drop this one then.

Don't worry. What build failure exactly? The lldb libc++ failures shown here in phab or something else?

Exactly, I was a bit lost by the lldb and libc++ failures

h-joo abandoned this revision.Jul 5 2020, 9:08 AM