Szelethus added a comment. Thanks for sticking this out! It just takes me a while to get to the mental space regarding this patch you are already in, sometimes through saying incorrect statements or stupid suggestions! :) If others could chip in in this discussion, it would be most appreciated, because I fear that otherwise this patch is a bit too exposed to my own wrongs.
In D75682#1918839 <https://reviews.llvm.org/D75682#1918839>, @balazske wrote: > In D75682#1917257 <https://reviews.llvm.org/D75682#1917257>, @Szelethus wrote: > > > Could you please add the warning we discussed? That would be a great > > demonstration of this patch's capabilities, and wouldn't deserve its own > > patch. > > > Was it this warning? > > if (feof(F)) { // note: Assuming the condition is true > // note: Stream 'F' has the EOF flag set > fgets(F); // warning: Illegal stream operation on a stream that has EOF > set > } > > > The warning above should be get even if the `feof` call is missing because it > is still possible that the previous operation resulted in EOF state. What I meant to demonstrate (but failed to describe) is that if the condition of the `if` statement is `!feof(F)`, we **shouldn't** get the warning. > The checker does not work by "assuming feof is true" (this can be a later > feature, if the `F` is not tracked and a feof call is encountered). Value of > `feof` (more precisely: if any error happened) is fixed when the file > operation is called that causes the error. [...] And still that warning would > not be testable because `FEof` error state is never set in this patch. I suspect that at this point you are 2 steps ahead of me, but this is how I would imagine such a patch to look like: we //evaluate// `feof()` by splitting the state (this should be worth the cost, because why would we ever call `feof` if the state couldn't be EOF or non-EOF?), associate the state where its return value is true with the argument being marked as EOF, and the other as non-EOF. This obviously wouldn't be sufficient -- values retrieved from stream reading operations may be EOF themselves, which should make us set the appropriate state for the originating stream, but that should indeed be the topic of a followup patch. What is the goal here if not this? I have a suspicion I'm just not seeing the obvious here, but I don't get it just yet. > By the way, a file read is not invalid if any error is there, it simply fails > again. Oh, yea, silly me. You're totally right :) > Probably it would be better to make a full working prototype first, because > the design decisions made now can turn out to be wrong later when the new > functionality would be added and we do "not see the full picture" now. If you try to implement a warning just to see how this would turn out long term, that would indeed be a good idea. Feel free to upload them, if you prefer, as a WIP patch! ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:393-394 + + // Make the return value accordingly to the error. + State = State->assume(RetVal, (SS->*IsOfError)()); + assert(State && "Return value should not be constrained already."); ---------------- Is this correct? We set the `feof()` return value whether we know the state of the stream to be EOF, but in this case we would incorrectly mark it as non-EOF: ```lang=bash $ cat a.txt # contains a single newline $ cat test.cpp ``` ```lang=cpp #include <cstdio> void tryRead(FILE *F) { char C = fgetc(F); if (feof(F)) printf("The stream is EOF!\n"); else printf("The stream is good! Read '%c'\n", C); } int main() { FILE *F = fopen("a.txt", "r"); tryRead(F); tryRead(F); // Incorrectly mark F to be non-EOF } ``` ```lang=bash $ clang test.cpp -fsanitize=address && ./a.out The stream is good! Read ' ' The stream is EOF! ``` Wouldn't it be safer to assume it to be `true` if we **know** its EOF and do nothing otherwise? How about a state split? (I know this functions also handler FError, but you see what my point is.) ================ Comment at: clang/test/Analysis/stream-error.c:33-34 + int ch = fputc('a', F); + if (ch == EOF) { + // FIXME: fputc not handled by checker yet, should expect TRUE + clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}} ---------------- Sure, we don't, but the bigger issue here is that we wouldn't mark `F` to be in EOF even if we did handle it, we would also need to understand that `ch == EOF` is the telling sign. But that also ins't in the scope of this patch :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75682/new/ https://reviews.llvm.org/D75682 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
