Re: r333082 - Fix duplicate class template definitions problem

[Gábor Márton via cfe-commits]

Thanks a lot!

Gabor

On Thu, May 24, 2018 at 12:54 PM Hans Wennborg  wrote:

> On Wed, May 23, 2018 at 3:53 PM, Gabor Marton via cfe-commits
>  wrote:
> > Author: martong
> > Date: Wed May 23 06:53:36 2018
> > New Revision: 333082
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=333082&view=rev
> > Log:
> > Fix duplicate class template definitions problem
> >
> > Summary:
> > We fail to import a `ClassTemplateDecl` if the "To" context already
> > contains a definition and then a forward decl.  This is because
> > `localUncachedLookup` does not find the definition.  This is not a
> > lookup error, the parser behaves differently than assumed in the
> > importer code.  A `DeclContext` contains one DenseMap (`LookupPtr`)
> > which maps names to lists.  The list is a special list `StoredDeclsList`
> > which is optimized to have one element.  During building the initial
> > AST, the parser first adds the definition to the `DeclContext`.  Then
> > during parsing the second declaration (the forward decl) the parser
> > again calls `DeclContext::addDecl` but that will not add a new element
> > to the `StoredDeclsList` rarther it simply overwrites the old element
> > with the most recent one.  This patch fixes the error by finding the
> > definition in the redecl chain.  Added tests for the same issue with
> > `CXXRecordDecl` and with `ClassTemplateSpecializationDecl`.  These tests
> > pass and they pass because in `VisitRecordDecl` and in
> > `VisitClassTemplateSpecializationDecl` we already use
> > `D->getDefinition()` after the lookup.
> >
> > Reviewers: a.sidorin, xazax.hun, szepet
> >
> > Subscribers: rnkovacs, dkrupp, cfe-commits
> >
> > Differential Revision: https://reviews.llvm.org/D46950
>
> [..]
>
> > +TEST_P(ASTImporterTestBase,
> > +   ImportDefinitionOfClassIfThereIsAnExistingFwdDeclAndDefinition) {
> > +  Decl *ToTU = getToTuDecl(
> > +  R"(
> > +  struct B {
> > +void f();
> > +  };
> > +
> > +  struct B;
> > +  )",
> > +  Lang_CXX);
> > +  ASSERT_EQ(2u, DeclCounter().match(
> > +ToTU,
> cxxRecordDecl(hasParent(translationUnitDecl();
>
> This doesn't hold when targeting Windows, as Sema will add an implicit
> declaration of type_info, causing the matcher to return 3 instead of
> 2.
>
> I've committed r333170 to fix.
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r352055 - Fix failing buildbots

[Gábor Márton via cfe-commits]

Hi,

Thank you for catching this. I thought that the macros like __x86_64__ are
defined for the target. I just don't understand: If they are defined for
the host, that would mean we can't cross compile on the same host for
different targets, wouldn't it?

I couldn't find out which macros to use to get the target arch, so I see 2
possible solutions :
1. Create a new test binary for these two small tests and specify
explicitly the target. This seems overwhelming.
2. Simply remove those two test cases. This seems to be the simplest
solution.

Gábor


On Fri, 1 Feb 2019, 17:23 David Green  Hello
>
> Sorry for the late reply. I'm not sure this ifdef is quite correct. It
> will be testing the _host_ architecture, presuming the default target is
> the same. If they are different (for example if the default target is
> aarch64 on an x86 machine), the test will presumably still fail.
>
> I went looking through the buildbots and I think this hexagon bot builds
> that way:
> http://lab.llvm.org:8011/builders/clang-hexagon-elf/builds/22699
>
> Got any good suggestions how to fix it?
>
> Thanks,
> Dave
>
>
>
>
>
> Author: martong
> Date: Thu Jan 24 07:42:20 2019
> New Revision: 352055
>
> URL: http://llvm.org/viewvc/llvm-project?rev=352055&view=rev
> Log:
> Fix failing buildbots
>
> Related commit which caused the buildbots to fail:
> rL352050
>
> Modified:
> cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp
>
> Modified: cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp?rev=352055&r1=352054&r2=352055&view=diff
>
> ==
> --- cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp (original)
> +++ cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp Thu Jan 24
> 07:42:20 2019
> @@ -378,14 +378,17 @@ TEST_F(StructuralEquivalenceFunctionTest
>EXPECT_TRUE(testStructuralMatch(t));
>  }
>
> +// These calling conventions may not be available on certain platforms.
> +#if defined(__x86_64__) && defined(__linux__)
>  TEST_F(StructuralEquivalenceFunctionTest,
>  FunctionsWithDifferentCallingConventions) {
>auto t = makeNamedDecls(
> -  "__attribute__((fastcall)) void foo();",
> +  "__attribute__((preserve_all)) void foo();",
>"__attribute__((ms_abi))   void foo();",
>Lang_C);
>EXPECT_FALSE(testStructuralMatch(t));
>  }
> +#endif
>
>  TEST_F(StructuralEquivalenceFunctionTest,
> FunctionsWithDifferentSavedRegsAttr) {
>auto t = makeNamedDecls(
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r352055 - Fix failing buildbots

[Gábor Márton via cfe-commits]

Dave,

The idea to check explicitly the triple inside the test function is quite
convincing. Will you try to fix it that way? Or if it can wait a bit, this
will be my first thing to do on Monday.

Gábor

On Fri, 1 Feb 2019, 19:39 David Green  Hello
>
>
> I think, because this is a unit-test, the compile will happen for the host
> (x86_64 in this case). So the binary will still be x86_64.
>
>
> The compile that the test runs will pick up whatever the default target
> triple is (hexagon for the bot, aarch64 for us). I don't know a lot about
> these tests, but I presume that somewhere deep within testStructuralMatch
> or makeNamedDecls it will be picking this up and we can override it?
>
>
> ..
>
>
> Looking at it now, I think the Args to buildASTFromCodeWithArgs will
> allow specific targets to be used. I'm not sure the best way to get that
> information through to there, but something like this would work:
>
>
> diff --git a/unittests/AST/StructuralEquivalenceTest.cpp
> b/unittests/AST/StructuralEquivalenceTest.c
> index e6c289a..52dba5e 100644
> --- a/unittests/AST/StructuralEquivalenceTest.cpp
> +++ b/unittests/AST/StructuralEquivalenceTest.cpp
> @@ -28,6 +28,7 @@ struct StructuralEquivalenceTest : ::testing::Test {
>  this->Code0 = SrcCode0;
>  this->Code1 = SrcCode1;
>  ArgVector Args = getBasicRunOptionsForLanguage(Lang);
> +Args.push_back("--target=x86_64-unknown-linux-gnu");
>
>  const char *const InputFileName = "input.cc";
>
>
> I wouldn't recommend that, exactly, it would needlessly reduce the testing
> on other targets. And I think for the hexagon target the x86 backend will
> not even be registered I believe. Perhaps just something like this from
> another ASTMatchesNode test, to try and capture the same intent as the
> ifdefs:
>
> TEST_F(StructuralEquivalenceFunctionTest,
> FunctionsWithDifferentSavedRegsAttr) {
>
>   if (llvm::Triple(llvm::sys::getDefaultTargetTriple()).getArch() !=
> llvm::Triple::x86_64)
> return;
>   ...
>
>
> Dave
>
>
>
> > Hi,
> >
> > Thank you for catching this. I thought that the macros like __x86_64__
> are defined for the target. I just don't understand: If they are defined
> for the host, > that would mean we can't cross compile on the same host
> for different targets, wouldn't it?
> >
> > I couldn't find out which macros to use to get the target arch, so I
> see 2 possible solutions :
> > 1. Create a new test binary for these two small tests and specify
> explicitly the target. This seems overwhelming.
> > 2. Simply remove those two test cases. This seems to be the simplest
> solution.
> >
> > Gábor
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r352055 - Fix failing buildbots

[Gábor Márton via cfe-commits]

Thank you for taking care of this! As I see now the hexagon build bot is
happy and the other bots too.

Thanks again,
Gábor

On Sat, 2 Feb 2019, 09:34 David Green  Sounds good to me, easy enough for me to test here. And I'll count that as
> a review.
>
>
> I've given it a try in rC352956. We can see how that bot feels about it.
>
>
> Dave
>
> > Dave,
> >
> > The idea to check explicitly the triple inside the test function is
> quite convincing. Will you try to fix it that way? Or if it can wait a bit,
> this will be my first thing to do on Monday.
> >
> > Gábor
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D66951: [ASTImporter] Add comprehensive tests for ODR violation handling strategies

[Gábor Márton via cfe-commits]

Thanks, for reaching out to me.

I am looking into it.

Gabor

On Mon, Sep 23, 2019 at 5:44 PM Simon Pilgrim via Phabricator <
revi...@reviews.llvm.org> wrote:

> RKSimon added a comment.
>
> @martong This is failing on windows buildbots:
> http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/19808
>
>
> Repository:
>   rL LLVM
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D66951/new/
>
> https://reviews.llvm.org/D66951
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r335491 - Revert "[ASTImporter] Import the whole redecl chain of functions"

[Gábor Márton via cfe-commits]

Hi Nico,

Yes, I reverted because it broke one of the lldb build bots.
Next time I'll include the reason in the revert commit.

Gábor


On Mon, 25 Jun 2018, 22:50 Nico Weber,  wrote:

> When reverting things, please say why in the commit message. (In this
> case, apparently because it broke the lldb buildbots?)
>
> On Mon, Jun 25, 2018 at 12:30 PM Gabor Marton via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: martong
>> Date: Mon Jun 25 09:25:30 2018
>> New Revision: 335491
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=335491&view=rev
>> Log:
>> Revert "[ASTImporter] Import the whole redecl chain of functions"
>>
>> This reverts commit r335480.
>>
>> Modified:
>> cfe/trunk/include/clang/AST/ASTImporter.h
>> cfe/trunk/lib/AST/ASTImporter.cpp
>> cfe/trunk/lib/AST/DeclBase.cpp
>> cfe/trunk/test/ASTMerge/class/test.cpp
>> cfe/trunk/unittests/AST/ASTImporterTest.cpp
>>
>> Modified: cfe/trunk/include/clang/AST/ASTImporter.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTImporter.h?rev=335491&r1=335490&r2=335491&view=diff
>>
>> ==
>> --- cfe/trunk/include/clang/AST/ASTImporter.h (original)
>> +++ cfe/trunk/include/clang/AST/ASTImporter.h Mon Jun 25 09:25:30 2018
>> @@ -43,15 +43,6 @@ class TagDecl;
>>  class TypeSourceInfo;
>>  class Attr;
>>
>> -  // \brief Returns with a list of declarations started from the
>> canonical decl
>> -  // then followed by subsequent decls in the translation unit.
>> -  // This gives a canonical list for each entry in the redecl chain.
>> -  // `Decl::redecls()` gives a list of decls which always start from the
>> -  // previous decl and the next item is actually the previous item in
>> the order
>> -  // of source locations.  Thus, `Decl::redecls()` gives different lists
>> for
>> -  // the different entries in a given redecl chain.
>> -  llvm::SmallVector getCanonicalForwardRedeclChain(Decl* D);
>> -
>>/// Imports selected nodes from one AST context into another context,
>>/// merging AST nodes where appropriate.
>>class ASTImporter {
>>
>> Modified: cfe/trunk/lib/AST/ASTImporter.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTImporter.cpp?rev=335491&r1=335490&r2=335491&view=diff
>>
>> ==
>> --- cfe/trunk/lib/AST/ASTImporter.cpp (original)
>> +++ cfe/trunk/lib/AST/ASTImporter.cpp Mon Jun 25 09:25:30 2018
>> @@ -71,25 +71,6 @@
>>
>>  namespace clang {
>>
>> -  template 
>> -  SmallVector
>> -  getCanonicalForwardRedeclChain(Redeclarable* D) {
>> -SmallVector Redecls;
>> -for (auto *R : D->getFirstDecl()->redecls()) {
>> -  if (R != D->getFirstDecl())
>> -Redecls.push_back(R);
>> -}
>> -Redecls.push_back(D->getFirstDecl());
>> -std::reverse(Redecls.begin(), Redecls.end());
>> -return Redecls;
>> -  }
>> -
>> -  SmallVector getCanonicalForwardRedeclChain(Decl* D) {
>> -// Currently only FunctionDecl is supported
>> -auto FD = cast(D);
>> -return getCanonicalForwardRedeclChain(FD);
>> -  }
>> -
>>class ASTNodeImporter : public TypeVisitor,
>>public DeclVisitor,
>>public StmtVisitor {
>> @@ -214,12 +195,6 @@ namespace clang {
>>  const InContainerTy &Container,
>>  TemplateArgumentListInfo
>> &Result);
>>
>> -using TemplateArgsTy = SmallVector;
>> -using OptionalTemplateArgsTy = Optional;
>> -std::tuple
>> -ImportFunctionTemplateWithTemplateArgsFromSpecialization(
>> -FunctionDecl *FromFD);
>> -
>>  bool ImportTemplateInformation(FunctionDecl *FromFD, FunctionDecl
>> *ToFD);
>>
>>  bool IsStructuralMatch(RecordDecl *FromRecord, RecordDecl *ToRecord,
>> @@ -433,8 +408,6 @@ namespace clang {
>>
>>  // Importing overrides.
>>  void ImportOverrides(CXXMethodDecl *ToMethod, CXXMethodDecl
>> *FromMethod);
>> -
>> -FunctionDecl *FindFunctionTemplateSpecialization(FunctionDecl
>> *FromFD);
>>};
>>
>>  template 
>> @@ -464,25 +437,6 @@ bool ASTNodeImporter::ImportTemplateArgu
>>  From.arguments(), Result);
>>  }
>>
>> -std::tuple> ASTNodeImporter::OptionalTemplateArgsTy>
>>
>> -ASTNodeImporter::ImportFunctionTemplateWithTemplateArgsFromSpecialization(
>> -FunctionDecl *FromFD) {
>> -  assert(FromFD->getTemplatedKind() ==
>> - FunctionDecl::TK_FunctionTemplateSpecialization);
>> -  auto *FTSInfo = FromFD->getTemplateSpecializationInfo();
>> -  auto *Template = cast_or_null(
>> -  Importer.Import(FTSInfo->getTemplate()));
>> -
>> -  // Import template arguments.
>> -  auto TemplArgs = FTSInfo->TemplateArguments->asArray();
>> -  TemplateArgsTy ToTemplArgs;
>> -  if (ImportTemplateArguments(TemplArgs.data(), TemplArgs.size(),
>> -

Re: [PATCH] D86874: [analyzer] Fix ArrayBoundCheckerV2 false positive regarding size_t indexer

[Gábor Márton via cfe-commits]

Hi Balázs,

Since reviews.llvm.org is offline, I am sending my comments below, inline.
Thanks for your huge effort in explaining all this!

Overall, I have a feeling that this approach targets only one specific
case, which is fine. But I believe we should think about all the other
possible cases, so we could get rid of other false positives too:
1) In case of multidimensional arrays, there may be a symbolic value in any
dimension.
2) What if  there are more symbolic values in the dimensions.

Cheers,
Gábor

On Thu, Sep 3, 2020 at 4:52 PM Balázs Benics via Phabricator <
revi...@reviews.llvm.org> wrote:

> steakhal added a comment.
>
> In D86874#inline-803844 ,
> @martong wrote:
>
> > I really feel that this check would have a better place in the
> implementation of `eval`. This seems really counter-intuitive to do this
> stuff at the Checker's level. Is there any reason why we can't do this in
> `eval`?
> > `evalBinOpNN` could return with Unknown, and the state should remain
> unchanged. Am I missing something?
>
> Ah, sort of.
> To make everything clear, I have to provide examples, building-blocks,
> reasoning and stuff, so please don't be mad if it's too long.
> **I hope you have a warm cup of tee to read through this - seriously - you
> will need that!**
>
Man, this requires a warm lunch and then hot cups(!) of coffee. :D


>
> Diving in
> -
>
> It is really important to make a clear distinction between evaluating an
> expression according to the semantics of the //meta-language// or of the
> //object-language//, because we might get different answers for the same
> expression.
>
> In fact, `evalBinOpNN` evaluates expressions according to the semantics of
> the //object-language//.
>
> In our context, meta-language is //mathematics//, and the
> //object-language// is the semantics of the abstract machine of the c/c++
> language.
> In mathematics, there is no such thing as overflow or value ranges, unlike
> in C++.
>
> Let's see an example:
> Assuming that `x` is in range `[1,UINT_MAX]`.
> `x + 1` will be in range `[2,ULL_MAX+1]` in the //meta-language//.
> `x + 1` will be in range `[0,0]u[2,UINT_MAX]` or in `[2,UINT_MAX+1]`
> weather the type of `x` is capable representing the (+1) value and the
> overflow is well-defined or not.
>
> Another valuable comment is that non-contiguous ranges (range sets) are
> not really useful.
> Knowing that `x` is in range `[0,0]u[2,UINT_MAX]` doesn't help much if you
> want to prove that:
> `x < 5` holds for all possible interpretations of `x`.
> We can not disproof that either.
>
> So, overflow/underflow can really screw the value ranges, preventing us
> from evaluating expressions over relational operators.
> Which is technically accurate, but not satisfiable in all cases - like in
> the checker `ArrayBoundCheckerV2`.
>
> ---
>
> Before describing why is it so problematic in this checker, I should give
> an overview, how this checker works.
>
> Overview of the logic of the ArrayBoundCheckerV2
> 
>
> The checker checks `Location` accesses (aka. pointer dereferences).
> We construct the `RegionRawOffsetV2` object of the access (Which consists
> of the //base region//, and the symbolic //byte offset// expression of the
> access).
>
> Then we check, whether we access an element //preceding// the **first
> valid index** of the //base// memory region.
> In other words, we check if the //byte offset// symbolic expression is
> **less then** 0:
>
> - If YES:   Report that we access an out-of-bounds element.
> - If NO:Infer the range constraints on the symbol and add the
> constraint to make this array access valid.
> - If MAYBE: Infer and constrain, just as you would do in the previous case.
>
> Then we check, whether we access an element //exceeding// the **last valid
> index** of the memory region.
> In other words, we check if the //byte offset// symbolic expression is
> greater then or equal to the //extent// of the region:
>
> - If YES:   Report the bug...
> - If NO:Infer & constrain...
> - If MAYBE: Infer & constrain...
>
> However, in the checker, we don't check `byte offset < 0` directly.
> We //simplify// the left part of the `byte offset < 0` inequality by
> substracting/dividing both sides with the constant `C`, if the `byte
> offset` is a `SymintExpr` of `SymExpr OP C` over the plus or multiplication
> operator (OP).
> We do this //simplification// recursively.
> In the end, we will have a //symbolic root// (call it `RootSym`)
> expression and a `C'` constant over the right-hand side of the original
> relational operator.
> So, after the //simplification// process we get the `RootSym < C'`
> inequality, which we check.
>
Just for the record, this is in `getSimplifiedOffsets`.


>
> This //simplification// is the //infer// operation in the pseudo-code.
> And the //constrain// step is where we assume that the negation of
> `RootSym < C'` holds.
>

> **SPOILER