Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

flang2 code is terrible #1376

Closed
dmikushin opened this issue Jun 21, 2023 · 10 comments
Closed

flang2 code is terrible #1376

dmikushin opened this issue Jun 21, 2023 · 10 comments

Comments

@dmikushin
Copy link
Contributor

dmikushin commented Jun 21, 2023 •
edited

Guys, this code is really bad quality. Someone who wrote it does not really understand how to write C/C++ code. I'm sorry to say this, but look for yourself:

classic-flang                 | /opt/llvm/bin/clang++ -DGTEST_HAS_RTTI=0 -DIN_FLANG2 -DMMD -DNOVECTORIZE -DOMP_OFFLOAD_LLVM -DPGFLANG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/classic-flang/src/classic-flang/include -I/classic-flang/build/flang/include -I/opt/llvm/include -I/classic-flang/build/flang/tools/flang2/include -I/classic-flang/src/classic-flang/tools/shared -I/classic-flang/src/classic-flang/include/flang -I/classic-flang/src/classic-flang/lib/scutil -I/classic-flang/src/classic-flang/tools/flang2/flang2exe -I/classic-flang/build/flang/tools/flang2/flang2exe -I/classic-flang/src/classic-flang/tools/flang2/flang2exe/x86_64 -I/classic-flang/build/flang/tools/flang2/utils/symtab -I/classic-flang/build/flang/tools/flang2/utils/ilitp -I/classic-flang/build/flang/tools/flang2/utils/ilmtp -I/classic-flang/build/flang/tools/flang2/utils/machar -I/classic-flang/build/flang/tools/flang2/utils/upper -I/classic-flang/src/classic-flang/tools/shared/utils -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -std=c++11 -fno-common -Woverloaded-virtual -Wcast-qual -fno-strict-aliasing -pedantic -Wno-long-long -Wall -W -Wno-unused-parameter -Wwrite-strings -Werror -Wno-error=sign-compare -Wno-error=pointer-sign -Wno-error=incompatible-pointer-types -Wno-error=string-conversion -Wno-error=c99-extensions -Wno-error=deprecated-register -Wno-error=format -Wno-error=format-pedantic -Wno-error=tautological-constant-out-of-range-compare -Wno-error=switch -Wno-error=covered-switch-default -Wno-error=enum-compare -Wno-error=char-subscripts -Wno-error=array-bounds -Wno-error=fortify-source -Wno-error=comment -Wno-error=unused-but-set-variable -O3 -DNDEBUG  -fno-exceptions -fno-rtti -MD -MT tools/flang2/flang2exe/CMakeFiles/flang2.dir/outliner.cpp.o -MF tools/flang2/flang2exe/CMakeFiles/flang2.dir/outliner.cpp.o.d -o tools/flang2/flang2exe/CMakeFiles/flang2.dir/outliner.cpp.o -c /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp
classic-flang                 | In file included from /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:17:
classic-flang                 | In file included from /classic-flang/src/classic-flang/tools/flang2/flang2exe/llassem.h:14:
classic-flang                 | In file included from /classic-flang/src/classic-flang/tools/flang2/flang2exe/llutil.h:14:
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/ll_structure.h:697:12: warning: flexible array members are a C99 feature [-Wc99-extensions]
classic-flang                 |   LL_MDRef elem[];
classic-flang                 |            ^
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:154:6: warning: array index 233 is past the end of the array (that has type 'int[96]') [-Warray-bounds]
classic-flang                 |   if(DBGBIT(233, 0x100))
classic-flang                 |      ^      ~~~
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/gbldefs.h:36:23: note: expanded from macro 'DBGBIT'
classic-flang                 | #define DBGBIT(n, m) (flg.dbg[n] & m)
classic-flang                 |                       ^       ~
classic-flang                 | /classic-flang/src/classic-flang/tools/shared/utils/global.h:196:3: note: array 'dbg' declared here
classic-flang                 |   int dbg[96];
classic-flang                 |   ^
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:162:6: warning: array index 233 is past the end of the array (that has type 'int[96]') [-Warray-bounds]
classic-flang                 |   if(DBGBIT(233, 0x200)) {
classic-flang                 |      ^      ~~~
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/gbldefs.h:36:23: note: expanded from macro 'DBGBIT'
classic-flang                 | #define DBGBIT(n, m) (flg.dbg[n] & m)
classic-flang                 |                       ^       ~
classic-flang                 | /classic-flang/src/classic-flang/tools/shared/utils/global.h:196:3: note: array 'dbg' declared here
classic-flang                 |   int dbg[96];
classic-flang                 |   ^
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:241:21: warning: variable 'presptr' set but not used [-Wunused-but-set-variable]
classic-flang                 |   int nmems, count, presptr;
classic-flang                 |                     ^
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:398:10: warning: variable 'rg' set but not used [-Wunused-but-set-variable]
classic-flang                 |   int i, rg, argl, ilix;
classic-flang                 |          ^
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:780:6: warning: array index 233 is past the end of the array (that has type 'int[96]') [-Warray-bounds]
classic-flang                 |   if(DBGBIT(233,2))
classic-flang                 |      ^      ~~~
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/gbldefs.h:36:23: note: expanded from macro 'DBGBIT'
classic-flang                 | #define DBGBIT(n, m) (flg.dbg[n] & m)
classic-flang                 |                       ^       ~
classic-flang                 | /classic-flang/src/classic-flang/tools/shared/utils/global.h:196:3: note: array 'dbg' declared here
classic-flang                 |   int dbg[96];
classic-flang                 |   ^
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:997:28: warning: comparison of integers of different signs: 'SPTR' and 'unsigned int' [-Wsign-compare]
classic-flang                 |       if (sptr > 0 && sptr < stb.stg_avail) {
classic-flang                 |                       ~~~~ ^ ~~~~~~~~~~~~~
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:1028:11: warning: variable 'i' set but not used [-Wunused-but-set-variable]
classic-flang                 |   int nw, i;
classic-flang                 |           ^
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:1135:7: warning: variable 'nw' set but not used [-Wunused-but-set-variable]
classic-flang                 |   int nw, len, noplen;
classic-flang                 |       ^
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:1206:7: warning: variable 'nw' set but not used [-Wunused-but-set-variable]
classic-flang                 |   int nw;
classic-flang                 |       ^
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:1221:11: warning: variable 'i' set but not used [-Wunused-but-set-variable]
classic-flang                 |   int nw, i, tlen;
classic-flang                 |           ^
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:1646:7: warning: variable 'base' set but not used [-Wunused-but-set-variable]
classic-flang                 |   int base, count, ilix, newcount;
classic-flang                 |       ^
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:1778:7: warning: variable 'paramct' set but not used [-Wunused-but-set-variable]
classic-flang                 |   int paramct, dpdscp;
classic-flang                 |       ^
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:1850:15: warning: variable 'prev_func_sptr' set but not used [-Wunused-but-set-variable]
classic-flang                 |   static SPTR prev_func_sptr;
classic-flang                 |               ^
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:1860:6: warning: array index 233 is past the end of the array (that has type 'int[96]') [-Warray-bounds]
classic-flang                 |   if(DBGBIT(233,2) && gbl.currsub) {
classic-flang                 |      ^      ~~~
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/gbldefs.h:36:23: note: expanded from macro 'DBGBIT'
classic-flang                 | #define DBGBIT(n, m) (flg.dbg[n] & m)
classic-flang                 |                       ^       ~
classic-flang                 | /classic-flang/src/classic-flang/tools/shared/utils/global.h:196:3: note: array 'dbg' declared here
classic-flang                 |   int dbg[96];
classic-flang                 |   ^
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:2105:20: warning: variable 'nw' set but not used [-Wunused-but-set-variable]
classic-flang                 |   int len, noplen, nw;
classic-flang                 |                    ^
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:2173:7: warning: variable 'nw' set but not used [-Wunused-but-set-variable]
classic-flang                 |   int nw;
classic-flang                 |       ^
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:2343:12: error: variable length arrays are a C99 feature [-Werror,-Wvla-extension]
classic-flang                 |   int args[nargs];
classic-flang                 |            ^~~~~
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:2343:12: note: read of non-const variable 'nargs' is not allowed in a constant expression
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:2338:7: note: declared here
classic-flang                 |   int nargs, nme, ili, i;
classic-flang                 |       ^
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:2344:20: error: variable length arrays are a C99 feature [-Werror,-Wvla-extension]
classic-flang                 |   DTYPE arg_dtypes[nargs];
classic-flang                 |                    ^~~~~
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:2344:20: note: read of non-const variable 'nargs' is not allowed in a constant expression
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:2338:7: note: declared here
classic-flang                 |   int nargs, nme, ili, i;
classic-flang                 |       ^
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/outliner.cpp:2519:7: warning: variable 'n_args' set but not used [-Wunused-but-set-variable]
classic-flang                 |   int n_args = 0, max_nargs, i;
classic-flang                 |       ^
classic-flang                 | 18 warnings and 2 errors generated.
classic-flang                 | [7/1311] Building CXX object tools/flang2/flang2exe/CMakeFiles/flang2.dir/error.cpp.o
classic-flang                 | [8/1311] Building CXX object tools/flang2/flang2exe/CMakeFiles/flang2.dir/ompaccel.cpp.o
classic-flang                 | FAILED: tools/flang2/flang2exe/CMakeFiles/flang2.dir/ompaccel.cpp.o 
classic-flang                 | /opt/llvm/bin/clang++ -DGTEST_HAS_RTTI=0 -DIN_FLANG2 -DMMD -DNOVECTORIZE -DOMP_OFFLOAD_LLVM -DPGFLANG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/classic-flang/src/classic-flang/include -I/classic-flang/build/flang/include -I/opt/llvm/include -I/classic-flang/build/flang/tools/flang2/include -I/classic-flang/src/classic-flang/tools/shared -I/classic-flang/src/classic-flang/include/flang -I/classic-flang/src/classic-flang/lib/scutil -I/classic-flang/src/classic-flang/tools/flang2/flang2exe -I/classic-flang/build/flang/tools/flang2/flang2exe -I/classic-flang/src/classic-flang/tools/flang2/flang2exe/x86_64 -I/classic-flang/build/flang/tools/flang2/utils/symtab -I/classic-flang/build/flang/tools/flang2/utils/ilitp -I/classic-flang/build/flang/tools/flang2/utils/ilmtp -I/classic-flang/build/flang/tools/flang2/utils/machar -I/classic-flang/build/flang/tools/flang2/utils/upper -I/classic-flang/src/classic-flang/tools/shared/utils -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -std=c++11 -fno-common -Woverloaded-virtual -Wcast-qual -fno-strict-aliasing -pedantic -Wno-long-long -Wall -W -Wno-unused-parameter -Wwrite-strings -Werror -Wno-error=sign-compare -Wno-error=pointer-sign -Wno-error=incompatible-pointer-types -Wno-error=string-conversion -Wno-error=c99-extensions -Wno-error=deprecated-register -Wno-error=format -Wno-error=format-pedantic -Wno-error=tautological-constant-out-of-range-compare -Wno-error=switch -Wno-error=covered-switch-default -Wno-error=enum-compare -Wno-error=char-subscripts -Wno-error=array-bounds -Wno-error=fortify-source -Wno-error=comment -Wno-error=unused-but-set-variable -O3 -DNDEBUG  -fno-exceptions -fno-rtti -MD -MT tools/flang2/flang2exe/CMakeFiles/flang2.dir/ompaccel.cpp.o -MF tools/flang2/flang2exe/CMakeFiles/flang2.dir/ompaccel.cpp.o.d -o tools/flang2/flang2exe/CMakeFiles/flang2.dir/ompaccel.cpp.o -c /classic-flang/src/classic-flang/tools/flang2/flang2exe/ompaccel.cpp
classic-flang                 | In file included from /classic-flang/src/classic-flang/tools/flang2/flang2exe/ompaccel.cpp:26:
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/ll_structure.h:697:12: warning: flexible array members are a C99 feature [-Wc99-extensions]
classic-flang                 |   LL_MDRef elem[];
classic-flang                 |            ^
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/ompaccel.cpp:639:1: error: non-void function does not return a value in all control paths [-Werror,-Wreturn-type]
classic-flang                 | }
classic-flang                 | ^
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/ompaccel.cpp:794:1: error: non-void function does not return a value [-Werror,-Wreturn-type]
classic-flang                 | }
classic-flang                 | ^
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/ompaccel.cpp:1223:9: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
classic-flang                 |   if (u >= stb.stg_avail)
classic-flang                 |       ~ ^  ~~~~~~~~~~~~~
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/ompaccel.cpp:1350:60: warning: variable 'sptrLaneOffset' set but not used [-Wunused-but-set-variable]
classic-flang                 |   SPTR sptrFn, sptrRhs, sptrReduceData, sptrShuffleReturn, sptrLaneOffset,
classic-flang                 |                                                            ^
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/ompaccel.cpp:1825:7: error: variable 'sptr' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
classic-flang                 |   if (outlinedCnt == 1) {
classic-flang                 |       ^~~~~~~~~~~~~~~~
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/ompaccel.cpp:1840:22: note: uninitialized use occurs here
classic-flang                 |   *targetfunc_sptr = sptr;
classic-flang                 |                      ^~~~
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/ompaccel.cpp:1825:3: note: remove the 'if' if its condition is always true
classic-flang                 |   if (outlinedCnt == 1) {
classic-flang                 |   ^~~~~~~~~~~~~~~~~~~~~~
classic-flang                 | /classic-flang/src/classic-flang/tools/flang2/flang2exe/ompaccel.cpp:1809:3: note: variable 'sptr' is declared here
classic-flang                 |   SPTR sptr;
classic-flang                 |   ^
classic-flang                 | 3 warnings and 3 errors generated.
classic-flang                 | [9/1311] Building CXX object tools/flang2/flang2exe/CMakeFiles/flang2.dir/ccffinfo.cpp.o
classic-flang                 | /classic-flang/build/flang/tools/flang2/flang2exe/ccffinfo.cpp:540:62: warning: format specifies type 'void *' but the argument has type 'MESSAGE *' (aka 'ccff_message *') [-Wformat-pedantic]
classic-flang                 |   fprintf(dfile, "ccff:%p type:%d lineno:%d order:%d id:%s", m, m->msgtype,
classic-flang                 |                        ~~                                    ^
classic-flang                 | /classic-flang/build/flang/tools/flang2/flang2exe/ccffinfo.cpp:2057:9: warning: variable 'ofile' set but not used [-Wunused-but-set-variable]
classic-flang                 |   FILE *ofile = NULL;
classic-flang                 |         ^
classic-flang                 | /classic-flang/build/flang/tools/flang2/flang2exe/ccffinfo.cpp:2424:43: warning: format specifies type 'void *' but the argument has type 'MESSAGE *' (aka 'ccff_message *') [-Wformat-pedantic]
classic-flang                 |     fprintf(gbl.dbgfil, ") returns %p\n", mptr);
classic-flang                 |                                    ~~     ^~~~
classic-flang                 | 3 warnings generated.
classic-flang                 | ninja: build stopped: subcommand failed.

Is this code really from PGI, NVIDIA and LLVM, not from a student homework? This has to be removed entirely, there is nothing to fix here.
@bryanpkc
Copy link
Collaborator

Classic Flang has a very long history; some of the code might have been automatically converted from another language into K&R C (C89 if we are lucky). An attempt was made to rewrite parts of it in C++ but that effort was abandoned. We do know that the code base works, works with language conformance tests and real applications, and is the basis of several commercial compilers. But if you need a code base that passes -pedantic checks, my recommendation for you is to use and contribute to LLVM Flang.

@bryanpkc
Copy link
Collaborator

If you change your mind and want to help fix these compiler warnings, feel free to submit PRs!

@ppenzin
Copy link

ppenzin commented Apr 8, 2024

Why were .c files simply renamed to .cpp? 😱 It breaks history and gives a false impression (which lead to the OP here for example) that the code is really C++.

@bryanpkc
Copy link
Collaborator

@gklimowicz @pawosm-arm Any idea?

@pawosm-arm
Copy link
Collaborator

@ppenzin it was a PGI's idea to improve the code quality: the more demanding C++ compiler in theory should expose existing flaws in the plain C code.

@ppenzin
Copy link

ppenzin commented Apr 17, 2024

What specific flaws of a C code base can be exposed by treating it as C++? Even though syntax is related, there are extra semantics in C++ which can lead to hard to follow bugs or simply heaps of warnings from what used to be perfectly fine code.

@gklimowicz
Copy link
Contributor

As @pawosm-arm mentioned, this was the process we had for migrating changes from PGI Fortran to Flang (that you helped create, Petr). I haven't been with NVIDIA for more than three years now, and the commit you mentioned is over 5-1/2 years old. But my recollection was that it was made to improve the type-checking in the back-end, and possibly to improve interfacing to the LLVM code generation libraries.

I am surprised that the change lost history in this repo. I thought git renames were treated better with respect to this kind of change.

@bryanpkc
Copy link
Collaborator

@gklimowicz History isn't lost, it is just a bit hidden. Running git log --follow on the renamed files will show the history of the files all the way up to the initial source drop. GitHub's history view will show a link at a renaming commit, letting you browse the history of the file before the rename (see this example).

@gklimowicz
Copy link
Contributor

@bryanpkc Thanks for that info. I haven’t needed to do this in the past, so I appreciate knowing (a) it can be done, and (b) how.

@pawosm-arm
Copy link
Collaborator

We all know how this code is and that there is plenty room for the improvements. Contributions are more than welcomed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@gklimowicz @ppenzin @dmikushin @bryanpkc @pawosm-arm