chrisburel / smokeqt

Other
1 stars 3 forks source link

Building smokeqt causes smokegen to assert #1

Open stcrocco opened 5 years ago

stcrocco commented 5 years ago

When building smokeqt, running make produces the following assertion from smokegen right at the beginning:

smokegen - debug(from unknown:0): using generator "/usr/bin/../lib/smokegen/generator_smoke.so"
smokegen - warning(from unknown:0): didn't find file "/home/stefano/documenti/programmi/qtruby/smokeqt/build1/qtdefines"
smokegen - debug(from unknown:0): parsing "/home/stefano/documenti/programmi/qtruby/smokeqt/qtcore/qtcore_includes.h"
smokegen: /usr/lib/llvm/8/include/llvm/Support/Casting.h:255: typename llvm::cast_retty<X, Y*>::ret_type llvm::cast(Y*) [with X = clang::NamedDecl; Y = clang::DeclContext; typename llvm::cast_retty<X, Y*>::ret_type = clang::NamedDecl*]: Assertion `isa<X>(Val) && "cast<Ty>() argument of incompatible type!"' failed.

In the past, I've successfully built it (as far as I can tell, last time I built it was in October 2018). I tried it again to install on another PC but now it fails. Unfortunately, I can't find out which version of llvm and clang I had the last time it build successfully: I'm sure it was 7., but I can tell what was. I tried to rebuild smokegen and smokeqt with clang 7.1.0 but it failed in the same way. I'll try to build it with clang 7.0.1, bu it'll take time because it's no longer availlable in my distribution, so I'll have to find out the best way to install it by hand.

If it matters, I had to modify the main CMakeLists.txt file for smokeqt because otherwise I got messages about missing include files. This happened even when smokeqt built successfully. This is what I had to add to the file at the beginning, just after the project(SMOKEQT5) line:

if(NOT(${APPLE}))
  execute_process(COMMAND "sh" "-c" "g++ -E -Wp,-v -xc++ /dev/null 2>&1 1>/dev/null | grep '^\\s'" OUTPUT_VARIABLE GCC_INC_LINES)
  string(REGEX REPLACE "\n" ";" GCC_INC_LINES "${GCC_INC_LINES}")
  list(FILTER GCC_INC_LINES EXCLUDE REGEX "^[ \n\t]*$")
  foreach(INC IN LISTS GCC_INC_LINES)
    string(STRIP ${INC} INC)
    if (IS_DIRECTORY "${INC}")
      list(APPEND GCC_INCLUDE ${INC})
    endif(IS_DIRECTORY "${INC}")
  endforeach(INC)
endif(NOT(${APPLE}))
stcrocco commented 5 years ago

I tried building smokegen using clang-7.0.1 but smokeqt still fails.

stcrocco commented 5 years ago

I investigated some more and find out that the issue isn't with the clang version but with the Qt version. The problem seems to arise from the file qcborcommon.h which is new in Qt 5.12 and, in particular, from QCborSimpleType::Undefined, on line 64 of that file.

The crash comes from line 13 of smokegen's defaultargvisitor.cpp. From what I understand, the line

 std::string prefix = clang::cast<clang::NamedDecl>(enumConstant->getDeclContext()->getParent())->getQualifiedNameAsString() + "::";

fails for that file, because enumConstant->getDeclContext()->getParent() is of class clang::TranslationUnitDecl, which is not an instance of clang::NamedDecl. I succeeded in making smokeqt compile by replacing that line with:

clang::NamedDecl *parent = clang::dyn_cast<clang::NamedDecl>(enumConstant->getDeclContext()->getParent());
if (parent) {
          std::string prefix = parent ->getQualifiedNameAsString() + "::";
          ...  
}

I haven't checked if the resulting library works or not however.

chrisburel commented 5 years ago

Thanks for the report.

It looks like this is a result of Qt preferring the new enum class feature. Previously to enum classes, it was poor form to put enums at the top level, since they would be added to the global namespace. So Qt put all their enums inside of a class, so they'd all be inside a NamedDecl. With enum classes, it is not a problem to place them at the top level.

I'll take a closer look and see if your fix works for all cases. The goal of the default arg visitor is to rewrite enums that are used to ensure the class they're defined in is prefixed to their name, so that the lookup will succeed in the generated files. Since enum classes always have to be qualified already, I think your fix is correct. If you'd like to make a pull request to the smokegen code, I could just merge it in.