29

It took me forever to track down that there was a bug in my code being triggered by /OPT:ICF:

Because /OPT:ICF can cause the same address to be assigned to different functions or read-only data members (const variables compiled by using /Gy), it can break a program that depends on unique addresses for functions or read-only data members.

(I had been storing and comparing function pointers for equality, which breaks when the linker throws away identical functions.)

Now I need to find every place where I might have done such a thing.

The test case is of course trivial:

//MSVC: /Gy /link /OPT:ICF
int test1(void) { return 0; }
int test2(void) { return 0; }
int main(void) { return test1 == test2; }

I've tried -Wall, -Wextra, -Weverything, -pedantic, etc. but none of them generate warnings.

Is there any compiler option or tool (whether part of Visual C++, GCC, Clang, or other) that can analyze my code and tell me where I'm comparing function pointers with each other, like in the code above?

Dean Seo
  • 5,486
  • 3
  • 30
  • 49
user541686
  • 205,094
  • 128
  • 528
  • 886
  • I love this question. Since it's up to the compiler to determine whether your comparison is true, you would like a diagnostic, yes? – Drew Dormann Jan 29 '18 at 05:17
  • 2
    @DrewDormann: Yes. I'm frankly flabbergasted that there doesn't already seem to be one as far as I can see. – user541686 Jan 29 '18 at 05:19
  • This appears to just be a tool recommendation question. It might help to just ask if there is a compiler option you can use. – Tas Jan 29 '18 at 05:19
  • @Tas: Edited... – user541686 Jan 29 '18 at 05:20
  • 1
    This is an invalid transformation the linker is making. – R.. GitHub STOP HELPING ICE Jan 29 '18 at 05:25
  • @R..: Interesting; citation? (though it wouldn't affect my question) – user541686 Jan 29 '18 at 05:27
  • You need software that will generate an abstract syntax tree for your code, and then you can find the comparisons on function pointers in the tree. – Sean F Jan 29 '18 at 05:34
  • 1
    Note MSVC is performing a non-conforming optimization see [Do distinct functions have distinct addresses?](https://stackoverflow.com/q/26533740/1708801) for details. Also see [Is Visual Studio 2013 optimizing correctly in the presence of /OPT:ICF?](https://stackoverflow.com/q/29056890/1708801) – Shafik Yaghmour Jan 29 '18 at 06:06
  • If you are ready to put in some effort, you can use [LibTooling](https://clang.llvm.org/docs/LibTooling.html) with clang to test for such cases. But ofcourse since your code is for MSVC, all the intrinsics and compiler extensions might not be supported. – Ajay Brahmakshatriya Jan 29 '18 at 06:07
  • I know that absolutely does not answer the question, but C/C++ and coding languages in general are made so the functions are discriminated by names, and so functions pointers pointing to the same place should be the same name by conception. If you need to check equality between function pointers to discriminate functions it means there is a conception issue somewhere. Or maybe I'm wrong and then I need some explanations. – Benjamin Barrois Jan 29 '18 at 06:14
  • 1
    @DrewDormann well what MSVC is doing is not a valid folding, so MSVC should just not do this. – Shafik Yaghmour Jan 29 '18 at 06:16
  • 4
    @BenjaminBarrois: If you'd like to ask "why would anyone ever think about comparing function pointers" then post it as a separate question and feel free to ping me and I'll be happy to post an answer. It's not relevant here though; I'm trying to remove them from my code, not add them. – user541686 Jan 29 '18 at 06:25
  • @Mehrdad That's an idea ;-) – Benjamin Barrois Jan 29 '18 at 06:28
  • @Mehrdad: wouldn't be deactivating the fusion of duplicated code in the linker a tweak (though not elegant and probably leading to execution inefficiency)? – Benjamin Barrois Jan 29 '18 at 06:55
  • @BenjaminBarrois: I don't understand your question. Are you asking if that would give me working code? Yes, but then that obviously means users of my code (including myself) can no longer use `/OPT:REF`. – user541686 Jan 29 '18 at 06:56
  • @Mehrdad: Yes they can if they also use `/OPT:NOICF`. – Benjamin Barrois Jan 29 '18 at 07:04
  • @BenjaminBarrois: Sigh, I (obviously) mistyped. I meant my users/myself can't use `/OPT:ICF` anymore. – user541686 Jan 29 '18 at 07:10
  • 1
    The OP deserves a big round of applause to spot this bug. Just amazing. – Dean Seo Jan 29 '18 at 07:41
  • 2
    @DeanSeo: Haha, thanks. It took me hours, and that was *with* my knowledge of what `/OPT:ICF` does. I just didn't realize it might be the culprit because I didn't think I was comparing function pointers anywhere, so it took me forever to realize I should try toggling it (and after that it was "merely" the pain of isolating the code). I can't imagine what another poor soul who doesn't know about the flag's behavior will have to go through to figure it out... – user541686 Jan 29 '18 at 07:47
  • 1
    @DeanSeo: P.S. I just noticed this is [deja-vu](https://stackoverflow.com/q/14176320/541686)... – user541686 Jan 29 '18 at 08:23
  • 1
    @Mehrdad should this question be closed as a duplicate then ? :p – Ajay Brahmakshatriya Jan 29 '18 at 10:09
  • 4
    @AjayBrahmakshatriya: No, not a duplicate. This specific question (generate warnings) addresses countermeasures, whereas the linked question talks about causes. – MSalters Jan 29 '18 at 15:37
  • 1
    @MSalters well since the MSVC optimization is non-conforming then why would any compiler issue a warning for this? If anything this should be a bug against MSVC or maybe if they don't want to fix it they could add the warning the OP wants as a work-around. – Shafik Yaghmour Jan 29 '18 at 17:27
  • 2
    @ShafikYaghmour: MSVC++ has a perfectly conforming `/OPT:NOICF` which is the default; you can't blame Microsoft for adding a fast-but-dangerous option. Same with fast floating point math - if you don't understand the trade-off, stick with the safe default. – MSalters Jan 29 '18 at 17:35
  • @MSalters sure, fair point but I guess my main point is it a reasonable expectation that any compiler besides maybe MSVC would produce a warning for this case. – Shafik Yaghmour Jan 29 '18 at 17:38
  • 1
    @Shafik Why would a **compiler** produce warnings for valid c++ code, just because it can cause troubles with some non-standard **linker** options? Does g++ produce warnings for this use case? – Voo Jan 29 '18 at 22:03
  • @Voo: Because it is the entity in the best position to do so? – user541686 Jan 29 '18 at 22:08
  • 2
    @Mehrdad That would mean producing warnings for perfectly valid c++ code, just because some other tool violates the standard. Certainly we could do so, but this really seems like something a separate tool would be preferable. Shouldn't be too hard with LLVM actually, I'm afraid MSVC is far behind the curve on these kind of things though. – Voo Jan 29 '18 at 22:13
  • I guess you can hack your copy of gcc to do so, if you really wish to go in this direction. – Serge Jan 30 '18 at 02:22
  • @ShafikYaghmour Well, one can (or could? I don't know whether they changed the relevant part of the standard) argue that folding those two functions is conforming. – Deduplicator Jan 30 '18 at 18:56

1 Answers1

9

Is there any compiler option or tool (whether part of Visual C++, GCC, Clang, or other) that can analyze my code and tell me where I'm comparing function pointers with each other, like in the code above?

I'm not sure if there exists such a compiler option.

However, there is such a tool. clang-tidy. You can write your own checks for clang-tidy, it's actually remarkably easy if you follow this blog. Specifically, the AST comes with a bunch of matchers already, which should handle the use-case you want.

Something like this seems to work:

binaryOperator(
    anyOf(hasOperatorName("=="), hasOperatorName("!=")),
    hasLHS(ignoringImpCasts(declRefExpr(hasType(functionType())))),
    hasRHS(ignoringImpCasts(declRefExpr(hasType(functionType())))))

Which flags the example in the OP:

fp.cxx:3:25: note: "root" binds here
int main(void) { return test1 == test2; }
                        ^~~~~~~~~~~~~~

That works specifically for the OP case, but you actually have to be more explicit to match all the other likely cases:

const auto AnyFunc = ignoringImpCasts(declRefExpr(hasType(anyOf(
    functionType(),
    pointsTo(functionType()),
    references(functionType())))));

Finder->AddMatcher(binaryOperator(
    anyOf(hasOperatorName("=="), hasOperatorName("!=")),
    hasLHS(AnyFunc),
    hasRHS(AnyFunc)).bind("op"), this);

Or something close to that effect.

Barry
  • 286,269
  • 29
  • 621
  • 977
  • 1
    Holy cow, I didn't realize just *how* easy it was to do this. I'll give this a try when I get a chance; thanks!! – user541686 Jan 30 '18 at 18:35