19 ms·

Clang-11.0.0 Miscompiled SQLite

302 points by creolabs 6y ago

adr_ 6y ago
Clang 11 hasn't been released yet, right?
- saagarjha 6y ago
  Nope, 10.0 was just released recently.
- loeg 6y ago
  Right. But we've also observed non-determinism / undefined behavior in Clang 10: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=246630#c26 https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=246630#c26 ==120363== Conditional jump or move depends on uninitialised value(s) ==120363== at 0x1634474: llvm::ConstantExpr::getGetElementPtr(llvm::Type*, llvm::Constant*, llvm::ArrayRef<llvm::Value*>, bool, llvm::Optional<unsigned int>, llvm::Type*) (Constants.cpp:2191) ==120363== by 0x112D6D9: getGetElementPtr (Constants.h:1163) ==120363== by 0x112D6D9: (anonymous namespace)::SymbolicallyEvaluateGEP(llvm::GEPOperator const*, llvm::ArrayRef<llvm::Constant*>, llvm::DataLayout const&, llvm::TargetLibraryInfo const*) (ConstantFolding.cpp:1005) ==120363== by 0x112DF70: (anonymous namespace)::ConstantFoldInstOperandsImpl(llvm::Value const*, unsigned int, llvm::ArrayRef<llvm::Constant*>, llvm::DataLayout const&, llvm::TargetLibraryInfo const*) (ConstantFolding.cpp:1039) ==120363== by 0x112C165: (anonymous namespace)::ConstantFoldConstantImpl(llvm::Constant const*, llvm::DataLayout const&, llvm::TargetLibraryInfo const*, llvm::SmallDenseMap<llvm::Constant*, llvm::Constant*, 4u, llvm::DenseMapInfo<llvm::Constant*>, llvm::detail::DenseMapPair<llvm::Constant*, llvm::Constant*> >&) [clone .part.0] (ConstantFolding.cpp:1114) ==120363== by 0x112C5CF: llvm::ConstantFoldConstant(llvm::Constant const*, llvm::DataLayout const&, llvm::TargetLibraryInfo const*) (ConstantFolding.cpp:1194) ==120363== by 0x188F410: prepareICWorklistFromFunction (InstructionCombining.cpp:3584) ==120363== by 0x188F410: combineInstructionsOverFunction(llvm::Function&, llvm::InstCombineWorklist&, llvm::AAResults*, llvm::AssumptionCache&, llvm::TargetLibraryInfo&, llvm::DominatorTree&, llvm::OptimizationRemarkEmitter&, llvm::BlockFrequencyInfo*, llvm::ProfileSummaryInfo*, unsigned int, llvm::LoopInfo*) (InstructionCombining.cpp:3703) ==120363== by 0x189205F: runOnFunction (InstructionCombining.cpp:3789) ==120363== by 0x189205F: llvm::InstructionCombiningPass::runOnFunction(llvm::Function&) (InstructionCombining.cpp:3768) ==120363== by 0x16F4352: llvm::FPPassManager::runOnFunction(llvm::Function&) (LegacyPassManager.cpp:1482) ==120363== by 0x16F4DE8: llvm::FPPassManager::runOnModule(llvm::Module&) (LegacyPassManager.cpp:1518) ==120363== by 0x16F51A2: runOnModule (LegacyPassManager.cpp:1583) ==120363== by 0x16F51A2: llvm::legacy::PassManagerImpl::run(llvm::Module&) (LegacyPassManager.cpp:1695) ==120363== by 0x1FF4CFE: EmitAssembly (BackendUtil.cpp:954) ==120363== by 0x1FF4CFE: clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::DataLayout const&, llvm::Module*, clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) (BackendUtil.cpp:1677) ==120363== by 0x2C471A8: clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) (CodeGenAction.cpp:335) ==120363== Uninitialised value was created by a stack allocation ==120363== at 0x112C653: (anonymous namespace)::SymbolicallyEvaluateGEP(llvm::GEPOperator const*, llvm::ArrayRef<llvm::Constant*>, llvm::DataLayout const&, llvm::TargetLibraryInfo const*) (ConstantFolding.c
  - saagarjha 6y ago
    I’m curious, does clang promise to compile deterministically? (This is inspired by your bug but not directly relevant here, it seems?)
    - Ace17 6y ago
      Non-deterministic compilation would be pretty bad. First, producing different outputs from the same input brings the question of caching tools (e.g ccache, FastBuild), who assume the compiler is a pure function of its inputs. Moreover, assuming your code and the compiler are correct, you might still end-up with a situation where the performance of the resulting binary differs depending on the planet's alignment at build time. Worse: when the input code is wrong (which does happens when you're writing new code and trying it on your machine) : you build your code locally, and you're in "luck", as the compiler generates binary code "that won't crash". So you push your modifications, and then you start getting complaints from your coworkers, because they pulled your commit and now they're getting crashes. At this moment you don't know what's happening yet, so you might even tell them "it works on my machine, did you try to rebuild all?". And this might appear to solve the issue, if this time your coworkers are "lucky"! Finally, let's suppose your code is correct, but the compiler has a code generation bug. Have you ever tried, as a user, to diagnose a compiler bug? You spent many hours trying to minimize the input file that triggers the bug, so it's executable without needing the rest of your project, so you can send it to the compiler devs. I wouldn't even try to do this if I knew the compiler was non-deterministic. In short, non-deterministic compilation is an invitation for trouble and confusion.
      - saagarjha 6y ago
        > First, producing different outputs from the same input brings the question of caching tools (e.g ccache, FastBuild), who assume the compiler is a pure function of its inputs. Why should they have to? Shouldn’t they just be able to reach for any valid compilation of this particular object file and slot it in? > Moreover, assuming your code and the compiler are correct, you might still end-up with a situation where the performance of the resulting binary differs depending on the planet's alignment at build time. This is already the case due to your environment. If you have the wrong number of environment variables you might penalize your program’s performance by a significant amount already just because you misalign the stack! > At this moment you don't know what's happening yet, so you might even tell them "it works on my machine, did you try to rebuild all?". And this might appear to solve the issue, if this time your coworkers are "lucky"! This sounds like the situation already with nondeterministic bugs like races, albeit with the same binary? > Have you ever tried, as a user, to diagnose a compiler bug? You spent many hours trying to minimize the input file that triggers the bug, so it's executable without needing the rest of your project, so you can send it to the compiler devs. I deal with nondeterministic programs all the time…they’re a bit more difficult to file bugs for, but it’s still possible.
- Twirrim 6y ago
  Why is OSSFuzz using such a bleeding edge compiler? That seems a little nuts.
  - bradfitz 6y ago
    Wouldn't you rather catch bugs before they're released in a stable version?
    - zamalek 6y ago
      Exactly. This is precisely the point of nightly builds, is it not?
      - Twirrim 6y ago
        Clang 11 is still in early development stages. Release date is several months away. Clang 10 was released just a couple of months ago. 11 is expected to be buggy and not fit for use yet. The SQLite devs now have to deal with "is it or isn't it a compiler bug" nonsense, taking their time away from fixing actual problems, working on features etc, from OSSFuzz deciding to use a compiler that the compiler devs themselves don't think is fit for use. How much trust can you have that even fuzz results exposed are actually legitimate either? False positives, or worse still false negatives?
    - aptqwa 6y ago
      It is unfair to the authors of the software that is actually tested, in this case SQLite. You are forced to investigate, otherwise people will attribute the bug to your software. Toolchain bugs take an amazing amount of time and energy and happen more often than people think.
    - 0xffff2 6y ago
      If you're going to go down that route, I would expect that they test using both the latest stable version and the whatever unstable version they want. Bugs found using the stable compiler should be reported to the project, while bugs found only using the unstable version should be reported to the compiler.
deleted 6y ago
[deleted]
mehrdadn 6y ago
Question for any low-level optimizing compiler engineers here: I obviously realize these are all important, but in your judgment, how much of making an error-free compiler would you say is about having a comprehensive test suite, vs. having very careful software engineers, vs. having extremely thorough code reviews, vs. something else? Put another way, if you were to lose one of these, which ones do you think would have the most/least negative impact (or be the easiest/hardest to make up for with other things) in terms of the correctness of the final product?
- neeeeees 6y ago
  The test suite is ultra important. Even in the unlikely case that you have an engineer who thoroughly understands the machine and language specs, you have a ton of existing, useful, programs that rely on under-defined behaviour in one or both of them.
  - mehrdadn 6y ago
    Ah, I might have to tease out my question here. By correctness I'm only referring to the implementation being true to the language specification, not to "not breaking existing programs". So if a program relies on undefined behavior and then gets broken in the next version of the compiler because of that behavior, I don't consider that a correctness problem in the compiler for the purposes of this question. (Which is not to suggest keeping it working isn't a worthwhile goal. I'm saying that's not the aspect of it that I'm asking about.)
    - pjmlp 6y ago
      The problem is that many don't learn C and C++ properly, they have read some language introduction book and assume "how my compiler does it" is the right way and probably never touched a copy of ISO. So plenty of programs rely on UB even without realizing it.
      - fit2rule 6y ago
        More to the point, anyone doing compiler validation work would a) always have a full test suite as their first line of attack, b) always have a well understood developer pipeline in place, to ensure a consistent understanding - coding rules, code review processes, etc. and c) always looks at what the compiler is actually doing, compared to what they think its doing - i.e. is able to navigate code -> assembly and all the stages in between, with competence. Anything less is asking for trouble.
aidenn0 6y ago
Given that it's SQLite, this is likely a compiler bug. However, the code given is insufficient to demonstrate a compiler bug. Given: c = pMem->flags; sqlite3VdbeMemRelease(pMem); pMem->flags = MEM_Str|MEM_Term|(c&(MEM_AffMask|MEM_Subtype)); You could have say: sqlite3VdbeMemRelease(struct foo pMem) { *(some_other_type *)&pMem->flags = bar; } In which case the C aliasing rules would allow the compiler to assume that the assignment through "some_other_type" does not affect the assignment through whatever type pMem->flags is. I have seen this bug happen before, when compilers got better at inlining, where it was something like void getAddressOfSomething(intptr_t *address); ... char *p; getAddressOfSomething((intptr_t *)&p) *p=foo The compiler could reorder the \*p=foo line to be before the getAddressOfSomething call for the same reason. TL;DR: Turn of strict aliasing via compiler flags (-fno-strict-aliasing on gcc) if you ever type-pun anywhere without using a union.
- chx 6y ago
  > Turn of strict aliasing via compiler flags (-fno-strict-aliasing on gcc) if you ever type-pun anywhere without using a union. That's the topic of Gary Bernhardt's (of JS WAT and Execute Program fame) very important "A Case Study in Not Being A Jerk in Open Source". https://www.destroyallsoftware.com/blog/2018/a-case-study-in-not-being-a-jerk-in-open-source https://www.destroyallsoftware.com/blog/2018/a-case-study-in...
  - saagarjha 6y ago
    Linus actually matches the standard in that one, interestingly.
  - schlupa 6y ago
    What an utter bullshit article. If people can't see the technical merit of an article past the swear words, they should stop working and join their convent of choice. Weaklings.
    - saagarjha 6y ago
      I think that is the entire point of the article.
fctorial 6y ago
c = pMem->flags; sqlite3VdbeMemRelease(pMem); pMem->flags = MEM_Str|MEM_Term|(c&(MEM_AffMask|MEM_Subtype)); 'pMem->flags' is a u16[1]. Shouldn't it be copied to 'c'. How can 'sqlite3VdbeMemRelease' alter the value of 'c'. [1]https://github.com/smparkes/sqlite/blob/8caf9219240123fbe6cff67b1e0da778c62d7621/src/vdbeInt.h#L148 https://github.com/smparkes/sqlite/blob/8caf9219240123fbe6cf...
- aidenn0 6y ago
  pMem is a pointer; the contents of pMem will not be copied. [edit] I see what you're asking. The point is that the compiler changes: c = pMem->flags; sqlite3VdbeMemRelease(pMem); To something equivalent to: sqlite3VdbeMemRelease(pMem); c = pMem->flags; Which is correct only if sqlite3VdbMemRelease doesn't change pMem->flags
- bilalhusain 6y ago
  'sqlite3VdbeMemRelease' doesn't alter the value of 'c' but may alter the value of 'pmem->flags' which is what the compiler replaced 'c' with. from the post by Dr. Richard Hipp "From the -S output, it looks like Clang-11.0.0 is compiling these three lines as if there were written as:" sqlite3VdbeMemRelease(pMem); pMem->flags = MEM_Str|MEM_Term|(pMem->flags&(MEM_AffMask|MEM_Subtype));
- outsomnia 6y ago
  He said >> Clang seems to be assuming that the sqlite3VdbeMemRelease() function does not change the value of pMem->flags.
- amake 6y ago
  It doesn't. The compiler is moving the access to `pMem->flags` to come later in the sequence, after `pMem` has been modified by `sqlite3VdbeMemRelease`.
- raverbashing 6y ago
  You're right. But for some reason, it's thinking it can just reread the flags from the structure after that function, which has a non-const pointer void sqlite3VdbeMemRelease(Mem *p); I'll be waiting for the excuses of the "C standard evangelists" and the extreme optimization people on how the SQLite developers are wrong on this case and how it's funny to pull the rug under them.
mister_hn 6y ago
I don't understand trying to use the trunk version of a under-development compiler. Would you use the binaries built from an unstable compiler in production? Given that here we talk about SQLite, what's the advantage to use clang 11 instead of a previous version?
- anoncareer0212 6y ago
  there's 3 versions of this comment floating around and another 2 floating around asking why this wasn't discovered earlier combine both to find zen koan, test early and often
- viraptor 6y ago
  You're looking at the wrong side of sqlite usage. If you develop sqlite, why wouldn't you test it on every compiler/version combination you can get your hands on? Whether someone uses it in production is irrelevant.
  - Nitramp 6y ago
    Because you'll spend a lot of time chasing down bugs that are actually issues in the compiler. Unless you're trying to vet the compiler itself, this won't improve your software but cost you a lot of effort.
    - saagarjha 6y ago
      If you’re SQLite, you generally do care about popular compilers converting your code into something wrong because you care about your code doing something wrong in general. If there was a bug in Intel processors that made SQLite fault randomly during tests, I’m sure they’d be interested in that too.
      - Nitramp 6y ago
        Yes. Two things though: 1. Hopefully none of your users run an unreleased clang from head, so you're not using anything a user will run into. 2. Consider if it is it worth the effort to validate every commit pushed to head, or whether you'd get more out of your investment if you only tested releases that received some more vetting (e.g. nightlies).
klysm 6y ago
If I understand correctly, everything is working as intended: a fuzzer caught a bug in an unreleased version of clang. The title makes it sound like somebody fucked up pretty badly.
- JDevlieghere 6y ago
  Indeed, the commit hash reported in the version corresponds to something that landed on master at 11PM this Monday.
  - sitkack 6y ago
    Clang is the lucky one here, it has both SQLite and Dr Hipp on its side. Rust has Crater which allows new rustc builds to be run against the entire public crates.io dgraph. https://github.com/rust-lang/crater https://github.com/rust-lang/crater
    - zoomablemind 6y ago
      > Clang is the lucky one here, it has both SQLite and Dr Hipp on its side. Clang is lucky that OSSFuzz does testing of unreleased clang version on other OSS projects, like SQLite.
      - renewiltord 6y ago
        Love it. This is the dream of open-source.
- schlupa 6y ago
  It is a fuck up. Ignoring a sequence point (function call) is serious business. Probably a too agressive inlining.
  - saagarjha 6y ago
    Relax, it’s a bug in a prerelease version of a compiler that was caught early and fixed promptly. Sure, it’s a bug in an important part of the compiler, but there’s not need to call foul.
ridiculous_fish 6y ago
These sorts of bugs are more common than you may think! Here’s gcc miscompiling fish shell: https://github.com/fish-shell/fish-shell/issues/6962 https://github.com/fish-shell/fish-shell/issues/6962
- deleted 6y ago
  [deleted]
- gnulinux 6y ago
  These sort of things get very tricky. Years ago in college me and my team was building an operating system and we were 100% sure we found a GCC bug. It turned out to be a gdb bug (we were disassembling with gdb) instead that made us think it was a GCC bug, but it was actually a bug in our code. Very subtle.
hoseja 6y ago
So... if I'm understanding correctly, you're accessing released memory? How is this anything but undefined behaviour?
- jabl 6y ago
  If I understand correctly, sqlite3VdbeMemRelease doesn't free pMem nor pMem->flags, just some other fields in the pMem struct.
  - KMag 6y ago
    You're almost certainly reading that correctly, that pMem is a pointer to a pointer to a block of memory, plus some metadata for that block of memory, and sqlite3VdbeMemRelease either releases the memory back to the application-level pool, frees it back to the memory allocator (glibc malloc/jremalloc/tcmalloc/dmaloc/etc.), or directly brk()/mummap()s it directly back to the OS. I have no experience with sqlite internals, but it's pretty clearly doing some application-level accounting on top of the normal memory management.
- hoseja 6y ago
  So... apparently I'm not understanding correctly haha.
nurettin 6y ago
Why would OSFuzz send the bug report to SQLite instead of Clang?
- zmodem 6y ago
  OSSFuzz is an automated system.
  - klysm 6y ago
    More generally, bug reports frequently get filed to a project that uses the system that actually has the bug as dependency - even with non-automated systems.
pwagland 6y ago
I see several references in this thread to "a bug in Clang that is already fixed", but I can't see anywhere where anyone references this bug. Can anyone point to the bug report (and/or fix) in Clang?
- danhor 6y ago
  https://bugs.llvm.org/show_bug.cgi?id=46195 https://bugs.llvm.org/show_bug.cgi?id=46195, it's now linked in the sqlite bug
eyegor 6y ago
Holy cow, this happened at -O1. This doesn't seem like the sort of optimization that should be possible at such a low level. I've run into plenty of trouble with higher level optimization flags in compilers before, but this is wild.

302 points by creolabs 6y ago

adr_ 6y ago

Clang 11 hasn't been released yet, right?

saagarjha 6y ago
Nope, 10.0 was just released recently.
loeg 6y ago
Right. But we've also observed non-determinism / undefined behavior in Clang 10: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=246630#c26 https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=246630#c26 ==120363== Conditional jump or move depends on uninitialised value(s) ==120363== at 0x1634474: llvm::ConstantExpr::getGetElementPtr(llvm::Type*, llvm::Constant*, llvm::ArrayRef<llvm::Value*>, bool, llvm::Optional<unsigned int>, llvm::Type*) (Constants.cpp:2191) ==120363== by 0x112D6D9: getGetElementPtr (Constants.h:1163) ==120363== by 0x112D6D9: (anonymous namespace)::SymbolicallyEvaluateGEP(llvm::GEPOperator const*, llvm::ArrayRef<llvm::Constant*>, llvm::DataLayout const&, llvm::TargetLibraryInfo const*) (ConstantFolding.cpp:1005) ==120363== by 0x112DF70: (anonymous namespace)::ConstantFoldInstOperandsImpl(llvm::Value const*, unsigned int, llvm::ArrayRef<llvm::Constant*>, llvm::DataLayout const&, llvm::TargetLibraryInfo const*) (ConstantFolding.cpp:1039) ==120363== by 0x112C165: (anonymous namespace)::ConstantFoldConstantImpl(llvm::Constant const*, llvm::DataLayout const&, llvm::TargetLibraryInfo const*, llvm::SmallDenseMap<llvm::Constant*, llvm::Constant*, 4u, llvm::DenseMapInfo<llvm::Constant*>, llvm::detail::DenseMapPair<llvm::Constant*, llvm::Constant*> >&) [clone .part.0] (ConstantFolding.cpp:1114) ==120363== by 0x112C5CF: llvm::ConstantFoldConstant(llvm::Constant const*, llvm::DataLayout const&, llvm::TargetLibraryInfo const*) (ConstantFolding.cpp:1194) ==120363== by 0x188F410: prepareICWorklistFromFunction (InstructionCombining.cpp:3584) ==120363== by 0x188F410: combineInstructionsOverFunction(llvm::Function&, llvm::InstCombineWorklist&, llvm::AAResults*, llvm::AssumptionCache&, llvm::TargetLibraryInfo&, llvm::DominatorTree&, llvm::OptimizationRemarkEmitter&, llvm::BlockFrequencyInfo*, llvm::ProfileSummaryInfo*, unsigned int, llvm::LoopInfo*) (InstructionCombining.cpp:3703) ==120363== by 0x189205F: runOnFunction (InstructionCombining.cpp:3789) ==120363== by 0x189205F: llvm::InstructionCombiningPass::runOnFunction(llvm::Function&) (InstructionCombining.cpp:3768) ==120363== by 0x16F4352: llvm::FPPassManager::runOnFunction(llvm::Function&) (LegacyPassManager.cpp:1482) ==120363== by 0x16F4DE8: llvm::FPPassManager::runOnModule(llvm::Module&) (LegacyPassManager.cpp:1518) ==120363== by 0x16F51A2: runOnModule (LegacyPassManager.cpp:1583) ==120363== by 0x16F51A2: llvm::legacy::PassManagerImpl::run(llvm::Module&) (LegacyPassManager.cpp:1695) ==120363== by 0x1FF4CFE: EmitAssembly (BackendUtil.cpp:954) ==120363== by 0x1FF4CFE: clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::DataLayout const&, llvm::Module*, clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) (BackendUtil.cpp:1677) ==120363== by 0x2C471A8: clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) (CodeGenAction.cpp:335) ==120363== Uninitialised value was created by a stack allocation ==120363== at 0x112C653: (anonymous namespace)::SymbolicallyEvaluateGEP(llvm::GEPOperator const*, llvm::ArrayRef<llvm::Constant*>, llvm::DataLayout const&, llvm::TargetLibraryInfo const*) (ConstantFolding.c
- saagarjha 6y ago
  I’m curious, does clang promise to compile deterministically? (This is inspired by your bug but not directly relevant here, it seems?)
  - Ace17 6y ago
    Non-deterministic compilation would be pretty bad. First, producing different outputs from the same input brings the question of caching tools (e.g ccache, FastBuild), who assume the compiler is a pure function of its inputs. Moreover, assuming your code and the compiler are correct, you might still end-up with a situation where the performance of the resulting binary differs depending on the planet's alignment at build time. Worse: when the input code is wrong (which does happens when you're writing new code and trying it on your machine) : you build your code locally, and you're in "luck", as the compiler generates binary code "that won't crash". So you push your modifications, and then you start getting complaints from your coworkers, because they pulled your commit and now they're getting crashes. At this moment you don't know what's happening yet, so you might even tell them "it works on my machine, did you try to rebuild all?". And this might appear to solve the issue, if this time your coworkers are "lucky"! Finally, let's suppose your code is correct, but the compiler has a code generation bug. Have you ever tried, as a user, to diagnose a compiler bug? You spent many hours trying to minimize the input file that triggers the bug, so it's executable without needing the rest of your project, so you can send it to the compiler devs. I wouldn't even try to do this if I knew the compiler was non-deterministic. In short, non-deterministic compilation is an invitation for trouble and confusion.
    - saagarjha 6y ago
      > First, producing different outputs from the same input brings the question of caching tools (e.g ccache, FastBuild), who assume the compiler is a pure function of its inputs. Why should they have to? Shouldn’t they just be able to reach for any valid compilation of this particular object file and slot it in? > Moreover, assuming your code and the compiler are correct, you might still end-up with a situation where the performance of the resulting binary differs depending on the planet's alignment at build time. This is already the case due to your environment. If you have the wrong number of environment variables you might penalize your program’s performance by a significant amount already just because you misalign the stack! > At this moment you don't know what's happening yet, so you might even tell them "it works on my machine, did you try to rebuild all?". And this might appear to solve the issue, if this time your coworkers are "lucky"! This sounds like the situation already with nondeterministic bugs like races, albeit with the same binary? > Have you ever tried, as a user, to diagnose a compiler bug? You spent many hours trying to minimize the input file that triggers the bug, so it's executable without needing the rest of your project, so you can send it to the compiler devs. I deal with nondeterministic programs all the time…they’re a bit more difficult to file bugs for, but it’s still possible.
Twirrim 6y ago
Why is OSSFuzz using such a bleeding edge compiler? That seems a little nuts.
- bradfitz 6y ago
  Wouldn't you rather catch bugs before they're released in a stable version?
  - zamalek 6y ago
    Exactly. This is precisely the point of nightly builds, is it not?
    - Twirrim 6y ago
      Clang 11 is still in early development stages. Release date is several months away. Clang 10 was released just a couple of months ago. 11 is expected to be buggy and not fit for use yet. The SQLite devs now have to deal with "is it or isn't it a compiler bug" nonsense, taking their time away from fixing actual problems, working on features etc, from OSSFuzz deciding to use a compiler that the compiler devs themselves don't think is fit for use. How much trust can you have that even fuzz results exposed are actually legitimate either? False positives, or worse still false negatives?
  - aptqwa 6y ago
    It is unfair to the authors of the software that is actually tested, in this case SQLite. You are forced to investigate, otherwise people will attribute the bug to your software. Toolchain bugs take an amazing amount of time and energy and happen more often than people think.
  - 0xffff2 6y ago
    If you're going to go down that route, I would expect that they test using both the latest stable version and the whatever unstable version they want. Bugs found using the stable compiler should be reported to the project, while bugs found only using the unstable version should be reported to the compiler.

deleted 6y ago

[deleted]

mehrdadn 6y ago

Question for any low-level optimizing compiler engineers here: I obviously realize these are all important, but in your judgment, how much of making an error-free compiler would you say is about having a comprehensive test suite, vs. having very careful software engineers, vs. having extremely thorough code reviews, vs. something else? Put another way, if you were to lose one of these, which ones do you think would have the most/least negative impact (or be the easiest/hardest to make up for with other things) in terms of the correctness of the final product?

neeeeees 6y ago
The test suite is ultra important. Even in the unlikely case that you have an engineer who thoroughly understands the machine and language specs, you have a ton of existing, useful, programs that rely on under-defined behaviour in one or both of them.
- mehrdadn 6y ago
  Ah, I might have to tease out my question here. By correctness I'm only referring to the implementation being true to the language specification, not to "not breaking existing programs". So if a program relies on undefined behavior and then gets broken in the next version of the compiler because of that behavior, I don't consider that a correctness problem in the compiler for the purposes of this question. (Which is not to suggest keeping it working isn't a worthwhile goal. I'm saying that's not the aspect of it that I'm asking about.)
  - pjmlp 6y ago
    The problem is that many don't learn C and C++ properly, they have read some language introduction book and assume "how my compiler does it" is the right way and probably never touched a copy of ISO. So plenty of programs rely on UB even without realizing it.
    - fit2rule 6y ago
      More to the point, anyone doing compiler validation work would a) always have a full test suite as their first line of attack, b) always have a well understood developer pipeline in place, to ensure a consistent understanding - coding rules, code review processes, etc. and c) always looks at what the compiler is actually doing, compared to what they think its doing - i.e. is able to navigate code -> assembly and all the stages in between, with competence. Anything less is asking for trouble.

aidenn0 6y ago

Given that it's SQLite, this is likely a compiler bug. However, the code given is insufficient to demonstrate a compiler bug. Given: c = pMem->flags; sqlite3VdbeMemRelease(pMem); pMem->flags = MEM_Str|MEM_Term|(c&(MEM_AffMask|MEM_Subtype)); You could have say: sqlite3VdbeMemRelease(struct foo pMem) { *(some_other_type *)&pMem->flags = bar; } In which case the C aliasing rules would allow the compiler to assume that the assignment through "some_other_type" does not affect the assignment through whatever type pMem->flags is. I have seen this bug happen before, when compilers got better at inlining, where it was something like void getAddressOfSomething(intptr_t *address); ... char *p; getAddressOfSomething((intptr_t *)&p) *p=foo The compiler could reorder the \*p=foo line to be before the getAddressOfSomething call for the same reason. TL;DR: Turn of strict aliasing via compiler flags (-fno-strict-aliasing on gcc) if you ever type-pun anywhere without using a union.

chx 6y ago
> Turn of strict aliasing via compiler flags (-fno-strict-aliasing on gcc) if you ever type-pun anywhere without using a union. That's the topic of Gary Bernhardt's (of JS WAT and Execute Program fame) very important "A Case Study in Not Being A Jerk in Open Source". https://www.destroyallsoftware.com/blog/2018/a-case-study-in-not-being-a-jerk-in-open-source https://www.destroyallsoftware.com/blog/2018/a-case-study-in...
- saagarjha 6y ago
  Linus actually matches the standard in that one, interestingly.
- schlupa 6y ago
  What an utter bullshit article. If people can't see the technical merit of an article past the swear words, they should stop working and join their convent of choice. Weaklings.
  - saagarjha 6y ago
    I think that is the entire point of the article.

fctorial 6y ago

c = pMem->flags; sqlite3VdbeMemRelease(pMem); pMem->flags = MEM_Str|MEM_Term|(c&(MEM_AffMask|MEM_Subtype)); 'pMem->flags' is a u16[1]. Shouldn't it be copied to 'c'. How can 'sqlite3VdbeMemRelease' alter the value of 'c'. [1]https://github.com/smparkes/sqlite/blob/8caf9219240123fbe6cff67b1e0da778c62d7621/src/vdbeInt.h#L148 https://github.com/smparkes/sqlite/blob/8caf9219240123fbe6cf...

aidenn0 6y ago
pMem is a pointer; the contents of pMem will not be copied. [edit] I see what you're asking. The point is that the compiler changes: c = pMem->flags; sqlite3VdbeMemRelease(pMem); To something equivalent to: sqlite3VdbeMemRelease(pMem); c = pMem->flags; Which is correct only if sqlite3VdbMemRelease doesn't change pMem->flags
bilalhusain 6y ago
'sqlite3VdbeMemRelease' doesn't alter the value of 'c' but may alter the value of 'pmem->flags' which is what the compiler replaced 'c' with. from the post by Dr. Richard Hipp "From the -S output, it looks like Clang-11.0.0 is compiling these three lines as if there were written as:" sqlite3VdbeMemRelease(pMem); pMem->flags = MEM_Str|MEM_Term|(pMem->flags&(MEM_AffMask|MEM_Subtype));
outsomnia 6y ago
He said >> Clang seems to be assuming that the sqlite3VdbeMemRelease() function does not change the value of pMem->flags.
amake 6y ago
It doesn't. The compiler is moving the access to `pMem->flags` to come later in the sequence, after `pMem` has been modified by `sqlite3VdbeMemRelease`.
raverbashing 6y ago
You're right. But for some reason, it's thinking it can just reread the flags from the structure after that function, which has a non-const pointer void sqlite3VdbeMemRelease(Mem *p); I'll be waiting for the excuses of the "C standard evangelists" and the extreme optimization people on how the SQLite developers are wrong on this case and how it's funny to pull the rug under them.

mister_hn 6y ago

I don't understand trying to use the trunk version of a under-development compiler. Would you use the binaries built from an unstable compiler in production? Given that here we talk about SQLite, what's the advantage to use clang 11 instead of a previous version?

anoncareer0212 6y ago
there's 3 versions of this comment floating around and another 2 floating around asking why this wasn't discovered earlier combine both to find zen koan, test early and often
viraptor 6y ago
You're looking at the wrong side of sqlite usage. If you develop sqlite, why wouldn't you test it on every compiler/version combination you can get your hands on? Whether someone uses it in production is irrelevant.
- Nitramp 6y ago
  Because you'll spend a lot of time chasing down bugs that are actually issues in the compiler. Unless you're trying to vet the compiler itself, this won't improve your software but cost you a lot of effort.
  - saagarjha 6y ago
    If you’re SQLite, you generally do care about popular compilers converting your code into something wrong because you care about your code doing something wrong in general. If there was a bug in Intel processors that made SQLite fault randomly during tests, I’m sure they’d be interested in that too.
    - Nitramp 6y ago
      Yes. Two things though: 1. Hopefully none of your users run an unreleased clang from head, so you're not using anything a user will run into. 2. Consider if it is it worth the effort to validate every commit pushed to head, or whether you'd get more out of your investment if you only tested releases that received some more vetting (e.g. nightlies).

klysm 6y ago

If I understand correctly, everything is working as intended: a fuzzer caught a bug in an unreleased version of clang. The title makes it sound like somebody fucked up pretty badly.

JDevlieghere 6y ago
Indeed, the commit hash reported in the version corresponds to something that landed on master at 11PM this Monday.
- sitkack 6y ago
  Clang is the lucky one here, it has both SQLite and Dr Hipp on its side. Rust has Crater which allows new rustc builds to be run against the entire public crates.io dgraph. https://github.com/rust-lang/crater https://github.com/rust-lang/crater
  - zoomablemind 6y ago
    > Clang is the lucky one here, it has both SQLite and Dr Hipp on its side. Clang is lucky that OSSFuzz does testing of unreleased clang version on other OSS projects, like SQLite.
    - renewiltord 6y ago
      Love it. This is the dream of open-source.
schlupa 6y ago
It is a fuck up. Ignoring a sequence point (function call) is serious business. Probably a too agressive inlining.
- saagarjha 6y ago
  Relax, it’s a bug in a prerelease version of a compiler that was caught early and fixed promptly. Sure, it’s a bug in an important part of the compiler, but there’s not need to call foul.

ridiculous_fish 6y ago

These sorts of bugs are more common than you may think! Here’s gcc miscompiling fish shell: https://github.com/fish-shell/fish-shell/issues/6962 https://github.com/fish-shell/fish-shell/issues/6962

deleted 6y ago
[deleted]
gnulinux 6y ago
These sort of things get very tricky. Years ago in college me and my team was building an operating system and we were 100% sure we found a GCC bug. It turned out to be a gdb bug (we were disassembling with gdb) instead that made us think it was a GCC bug, but it was actually a bug in our code. Very subtle.

hoseja 6y ago

So... if I'm understanding correctly, you're accessing released memory? How is this anything but undefined behaviour?

jabl 6y ago
If I understand correctly, sqlite3VdbeMemRelease doesn't free pMem nor pMem->flags, just some other fields in the pMem struct.
- KMag 6y ago
  You're almost certainly reading that correctly, that pMem is a pointer to a pointer to a block of memory, plus some metadata for that block of memory, and sqlite3VdbeMemRelease either releases the memory back to the application-level pool, frees it back to the memory allocator (glibc malloc/jremalloc/tcmalloc/dmaloc/etc.), or directly brk()/mummap()s it directly back to the OS. I have no experience with sqlite internals, but it's pretty clearly doing some application-level accounting on top of the normal memory management.
hoseja 6y ago
So... apparently I'm not understanding correctly haha.

nurettin 6y ago

Why would OSFuzz send the bug report to SQLite instead of Clang?

zmodem 6y ago
OSSFuzz is an automated system.
- klysm 6y ago
  More generally, bug reports frequently get filed to a project that uses the system that actually has the bug as dependency - even with non-automated systems.

pwagland 6y ago

I see several references in this thread to "a bug in Clang that is already fixed", but I can't see anywhere where anyone references this bug. Can anyone point to the bug report (and/or fix) in Clang?

danhor 6y ago
https://bugs.llvm.org/show_bug.cgi?id=46195 https://bugs.llvm.org/show_bug.cgi?id=46195, it's now linked in the sqlite bug

eyegor 6y ago

Holy cow, this happened at -O1. This doesn't seem like the sort of optimization that should be possible at such a low level. I've run into plenty of trouble with higher level optimization flags in compilers before, but this is wild.