C code sample for interview



  • Today, for only the second time in my career, I had the opportunity to be on the giving side of the interview table. We were interviewing new college graduates for digital logic design/verification positions, but both claimed C experience on their resumes, so I gave them the following code, reproducing an actual bug I encountered at a previous company, and sample output to debug. Since this was a pencil-and-paper exercise -- they didn't have access to a running system with a debugger -- I told them the information they would have learned from the debugger if they asked, and eventually, even if they didn't -- in particular, that the segfault occured in the printf() call.

    Any opinions on whether the difficulty was appropriate? Both candidates got the general cause of the bug, but neither quite nailed it.

    #include <stdlib.h>
    #include <stdio.h>
    

    typedef unsigned long long int uint64;
    typedef enum {READ, WRITE, OTHER} Type;
    typedef struct
    {
    uint64 addr;
    uint64 data;
    Type type;
    int valid;
    } Transaction;

    extern Transaction* getTrans();
    extern void doRead(Transaction* t);
    extern void doWrite(Transaction* t);
    extern char* typeStr(Type t);

    int main()
    {
    uint64 addr, data;
    char *msg;
    Transaction *trans;

    for (trans = getTrans(); trans-&gt;valid; trans = getTrans())
    {
        addr = trans-&gt;addr;
        data = trans-&gt;data;
        switch (trans-&gt;type)
        {
            case READ:
                doRead(trans);
                break;
            case WRITE:
                doWrite(trans);
                break;
            default:
                msg = typeStr(trans-&gt;type);
                printf("ERROR: Address: %016x Data: %016x %s\n",
                        addr, data, msg);
                break;
        }
    }
    

    }

    The program behavior is different running under Linux and Windows (compiled using gcc under Cygwin). Interestingly, neither reproduces the behavior in the original version, which printed part of the message before segfaulting in the guts of libc (putc(), IIRC). That the program didn't segfault under the version of Linux/gcc we use was rather a bit of a surprise to me.

    Linux:

    Address: 327b23c66b8b4567 Data: 66334873643c9869 WRITE
    Address: 625558ec2ae8944a Data: 46e87ccd238e1f29 READ
    Address: 41b71efb2eb141f2 Data: 7545e14679e2a9e3 READ
    Address: 4db127f812200854 Data: 1f16e9e80216231b WRITE
    Address: 3352255a140e0f76 Data: 0ded7263109cf92e WRITE
    Address: 6b68079a41a7c4c9 Data: 25e45d324e6afb66 WRITE
    Address: 7c83e4583f2dba31 Data: 62bbd95a257130a3 WRITE
    Address: 721da317333ab105 Data: 2d1d5ae92443a858 READ
    ERROR: Address: 0000000008edbdab Data: 000000004353d0cd OTHER
    Address: 2ca8861171f32454 Data: 02901d820836c40e READ
    Address: 7c3dbd3d1e7ff521 Data: 6ceaf087737b8ddc READ
    Address: 614fd4a13006c83e Data: 5577f8e1419ac241 READ
    Address: 77465f013804823e Data: 5c482a977724c67e READ
    Address: 2d51779651ead36b Data: 153ea438580bd78f READ
    Address: 2a487cb06a2342ec Data: 725a06fb1d4ed43b READ
    Address: 4b588f547a6d8d3c Data: 6de91b18542289ec WRITE
    Address: 684a481a32fff902 Data: 749abb43579478fe WRITE
    Address: 75c6c33a79a1deaa Data: 70c6a52912e685fb WRITE
    ERROR: Address: 000000004f4ef005 Data: 00000000649bb77c OTHER
    Address: 235ba861180115be Data: 354fe9f947398c89 READ
    Address: 10233c990d34b6a8 Data: 615740953f6ab60f WRITE
    Address: 310c50b3579be4f1 Data: 2f305def5ff87e05 WRITE
    Address: 1f48eaa14ad084e9 Data: 5db70ae51381823a READ
    Address: 5f5e7fd015014acb Data: 799d0247098a3148 READ
    Address: 1eba5d23168e121f Data: 5dc79ea8661e3f1e READ
    Address: 613efdc551d9c564 Data: 11447b730bf72b14 READ
    Address: 1a32234b08f2b15e Data: 68eb2f633b0fd379 WRITE
    ERROR: Address: 0000000006a5ee64 Data: 000000007fffca11 OTHER
    Address: 06eb5bd47fb7e0aa Data: 094211f26f6dd9ac WRITE

    Windows:

    Address: 40b18ccf5851f42d Data: 470331294bb5f646 READ
    Address: 502959d81a8b7f78 Data: 6c0356a72b894868 WRITE
    Address: 28e4baf170a3a52b Data: 0ae16fd97d8341fc READ
    Address: 40f7702c76035e09 Data: 2aa841576fa72ca5 READ
    Address: 04185faf2e533cc4 Data: 0cab86286de3b115 READ
    Segmentation fault (core dumped)




    • What happens when doRead or doWrite fails?
    • Is the lack of a free(trans); a bug?
    • What does typeStr return when its argument is not OTHER?
    • Why are you even calling a function that always returns "OTHER"?
    • If it doesn't always return a statically allocated string "OTHER", what is it supposed to do?


  • If you'd given me that code to look at, my first question would have been "Can I see the source for typeStr()? Because I want to be convinced that the pointer it returns references memory that isn't going to get stomped on."

    And then I would have noticed the fact that the Linux sample output always reports zeroes in the top half of the addresses in ERROR: lines, and said "D'OH! That printf() format string should be using %p instead of %x, because as it stands right now, printf() is expecting ints to go with the %x format specifiers. If you use a compiler where int is 32 bits, pointers are 64 bits and arguments are passed on the stack rather than in registers, the %s specifier is going to end up trying to convert a string starting at *data, not *msg."



  • @flabdablet said:

    D'OH! That printf() format string should be using %p instead of %x

    But they're not pointers. They're unsigned long longs.



  • First, everything except the declaration of addr and data, and the call to printf(), is fluff so the candidates would have to look at more than just two lines, and actually search for the bug. @Ben L. said:

    • What happens when doRead or doWrite fails?

    For the purposes of this sample, I don't really care. All they actually do is print the same message as the error message, but without the "ERROR: ", and without the bug. BTW, there were two reasons for putting them in another file: 1) To remove (some, but not all) distraction from the relevant bug, and 2) so the difference in the printf() calls wouldn't make the bug blindingly obvious.@Ben L. said:

    • Is the lack of a free(trans); a bug?

    Yes, certainly, and bonus points to a candidate who noticed it, but not relevant to the illustrated failure. An even bigger bug, not evident from the sample, is that the quick-and-dirty getTrans() I wrote never returns a trans with valid == false; therefore, the for loop never terminates, and I had to ^C the program under Linux, since it didn't segfault.@Ben L. said:

    • What does typeStr return when its argument is not OTHER?

    It returns "READ", "WRITE" or "OTHER". doRead() and doWrite() also call it to produce their output messages.@Ben L. said:

    • Why are you even calling a function that always returns "OTHER"?

    Well, that would be kind of pointless, if it did that, but it doesn't.@Ben L. said:

    • If it doesn't always return a statically allocated string "OTHER", what is it supposed to do?

    Answered above, but thanks; I just noticed another thing that isn't relevant to the issue at hand. I really should have declared typeStr and msg to be const char*, since bad things would happen if the caller tried to modify the returned string.

     

     



  • @Ben L. said:

    @flabdablet said:
    D'OH! That printf() format string should be using %p instead of %x
    But they're not pointers. They're unsigned long longs.
    True, but he got the cause of the bug exactly right; he's just a little off on the solution -- and it would probably result in the correct value being printed, even if it's technically not quite right.



  • @HardwareGeek said:

    @Ben L. said:

    @flabdablet said:
    D'OH! That printf() format string should be using %p instead of %x

    But they're not pointers. They're unsigned long longs.
    True, but he got the cause of the bug exactly right; he's just a little off on the solution -- and it would probably result in the correct value being printed, even if it's technically not quite right.

    %llX, then? Kind of a weird behaviour, but maybe that's because I'm spoiled by .NET.



  • @Maciejasjmj said:

    @HardwareGeek said:

    @Ben L. said:

    @flabdablet said:
    D'OH! That printf() format string should be using %p instead of %x
    But they're not pointers. They're unsigned long longs.
    True, but he got the cause of the bug exactly right; he's just a little off on the solution -- and it would probably result in the correct value being printed, even if it's technically not quite right.

    %llX, then? Kind of a weird behaviour, but maybe that's because I'm spoiled by .NET.

    Yes, although I prefer abcdef over ABCDEF, so I'd use %llx rather than %llX. In the absence of a company coding standard or some really stupid case-sensitive parser that has to post-process the output (yeah, been there; done that), that's just my preference.

     



  • @Maciejasjmj said:

    @HardwareGeek said:

    @Ben L. said:

    @flabdablet said:
    D'OH! That printf() format string should be using %p instead of %x

    But they're not pointers. They're unsigned long longs.
    True, but he got the cause of the bug exactly right; he's just a little off on the solution -- and it would probably result in the correct value being printed, even if it's technically not quite right.

    %llX, then? Kind of a weird behaviour, but maybe that's because I'm spoiled by .NET.

    even Go gets it right.



  • @Ben L. said:

    @flabdablet said:
    D'OH! That printf() format string should be using %p instead of %x
    But they're not pointers. They're unsigned long longs.
    I just thought of something. "Address" in the context I or the candidates might encounter a bug like this would only be an address within a device or system being simulated, not a real address on the real computer running the simulation; therefore, it is a pure number, not a pointer. I'm so accustomed to thinking inside this particular box that it never occurred to me that it might confuse someone into thinking it was a pointer. I wonder if that confused the candidates...



  • @HardwareGeek said:

    Any opinions on whether the difficulty was appropriate? Both candidates got the general cause of the bug, but neither quite nailed it.

    Upon seeing the mention of segfault in printf, the use of 64-bit integers and the %016x format I immediately though of a mismatch between expected and passed parameter size. Is that right? I think the difficulty was appropriate; you wanted to get a difference between the candidates and you got it. It would only have been too hard if neither candidate was able to answer it at all (or conversely, too easy if both produced the perfect answer).

    @HardwareGeek said:

    The program behavior is different running under Linux and Windows (compiled using gcc under Cygwin). Interestingly, neither reproduces the behavior in the original version, which printed part of the message before segfaulting in the guts of libc (putc(), IIRC). That the program didn't segfault under the version of Linux/gcc we use was rather a bit of a surprise to me.

    You neglected to mention the bitness of these platforms, but I'll bet at least the Linux is 64-bit and the difference comes down to the different data models of the operating systems. Linux has an LP64 model, meaning that long and pointers are 64-bit, while plain int is 32-bit. Since integers get promoted to the long type when passed through a variadic argument list, printf is expecting all integer arguments to be at least 64 bits wide and the program works fine. Windows, on the other hand, has an LLP64 model, in which long is still 32-bit (they probably did this to avoid breaking badly written proprietary software that assumes particular bitness for types even though the standard only guarantees a minimum width). Thus it produces the same segfault in this situation as a 32-bit platform.

    As for segfaulting part of the way through the message, I suppose that's due to implementation differences. It would seem that your original version (either libc or the program itself) flushes stdout even on an abnormal termination, so the part of the message already processed comes through. You can get the same behavior on Linux at least by installing a signal handler for SIGSEGV that performs the flush and re-raises the signal.


  • Discourse touched me in a no-no place

    @flabdablet said:

    arguments are passed on the stack rather than in registers
    Arguments to printf() are always passed on the stack. Theoretically they don't have to be, but it is variable width so support for using the stack's going to be mandatory, and the level of hackery required to make the ABI handle that mixed case when the stack is easily available is going to persuade people to ignore doing the complicated thing.

    Either that or we've got an epic candidate for the front page. (Minus the pointless “human-interest” fluff.)



  • @HardwareGeek said:

    An even bigger bug, not evident from the sample, is that the quick-and-dirty getTrans() I wrote never returns a trans with valid == false; therefore, the for loop never terminates, and I had to ^C the program under Linux, since it didn't segfault.

    I was going to ask whether getTrans() even guarantees never to return NULL.



  • @HardwareGeek said:

    Any opinions on whether the difficulty was appropriate? Both candidates got the general cause of the bug, but neither quite nailed it.

    Depends on whether you think that to call yourself "C-Programmer", you need to know arcane details of the printf syntax without looking it up.



  • @derari said:

    @HardwareGeek said:
    Any opinions on whether the difficulty was appropriate? Both candidates got the general cause of the bug, but neither quite nailed it.

    Depends on whether you think that to call yourself "C-Programmer", you need to know arcane details of the printf syntax without looking it up.

    A "C Expert" should know that. A "C Programmer" probably wouldn't unless s/he has encountered this behaviour before. So I'd say that it would be a bit too difficult for fresh college grads, but fairly okay for someone who claims some experience.



  • @Maciejasjmj said:

    Filed under: if c++ did one thing right it's std::cout [b]but then fucked it up again by overriding << and >>[/b]

    FTFY


  • Discourse touched me in a no-no place

    @Hmmmm said:

    but then fucked it up again by overriding <>
    Hmm? My C++ is a bit rusty these days, but what are you talking about?



  • Inconsistent behaviour between platforms is hardly a surprise. Nobody has mentioned the reason, though. The words you are looking for are "undefined behaviour" and as you should all know that means "demons fly out of your nose" or "millions die in a nuclear fireball", although the latter applies only when the code is running on a DeathStation 9000.



  • I think the world would be a better place if it used PRIx64 from inttypes.h

    #include <stdio.h>
    #include <inttypes.h>
    

    int main (int argc , char ** argv)
    {
    uint64_t a = 0xC0FFEE;
    uint64_t b = 0xBADBEEF;

    printf("a: %" PRIx64 "\nb: %" PRIx64 "\n", a, b);
    
    return 0;
    

    }

    Output:

    a: c0ffee
    b: badbeef
    

    or did i miss something?



  • @Steve The Cynic said:

    Inconsistent behaviour between platforms is hardly a surprise. Nobody has mentioned the reason, though. The words you are looking for are "undefined behaviour" and as you should all know that means "demons fly out of your nose" or "millions die in a nuclear fireball", although the latter applies only when the code is running on a DeathStation 9000.

    Hence why inttypes.h exists. From the (linux) manpage:

    
    APPLICATION USAGE
           The purpose of <inttypes.h> is to provide a set of integer types whose definitions are consistent across
           machines  and independent of operating systems and other implementation idiosyncrasies.  It defines, via
           typedef, integer types of various sizes. Implementations are free to  typedef  them  as  ISO C  standard
           integer  types  or extensions that they support. Consistent use of this header will greatly increase the
           portability of applications across platforms.
    
    


  • The bug is in the makefile. The definition of CFLAGS is missing -Wall -Werror.


    I think it's a good test question, BTW.



  • @PJH said:

    @Hmmmm said:
    but then fucked it up again by overriding << & >>
    Hmm? My C++ is a bit rusty these days, but what are you talking about?

    FTFM



  • Good lord, there' so much fail in this thread.

    @Ben L. said:

    @flabdablet said:
    D'OH! That printf() format string should be using %p instead of %x
    But they're not pointers. They're unsigned long longs.
     

    Heresy.  From stdint.h on a Red Hat box I'm on:

    # if __WORDSIZE == 64
    typedef long int                int64_t;
    # else
    __extension__
    typedef long long int           int64_t;
    # endif

     @tdb said:

    Linux has an LP64 model, meaning that long and pointers are 64-bit, while plain int is 32-bit. Since integers get promoted to the long type when passed through a variadic argument list...

    Completely wrong.  shorts and chars get promoted to int when passed to variadic functions.  ints don't get promoted to anything

     @dkf said:

    Arguments to printf() are always passed on the stack. Theoretically they don't have to be, but it is variable width so support for using the stack's going to be mandatory, and the level of hackery required to make the ABI handle that mixed case when the stack is easily available is going to persuade people to ignore doing the complicated thing.

    Incorrect, at least for any OS following the amd64 ABI (which includes Linux and the BSDs).  Variadic functions follow the same calling convention as normal functions, which means that the first several arguments are passed in registers.  This, by the way, is the real reason why the sample program does not crash on x86-64 Linux.  The two uint64_t arguments and the pointer argument are all passed in 64-bit registers.  The same would happen even if 32-bit ints had been passed, so when the varargs handling code in the compiler pulls out the first 32-bit int (obeying the format string), it knows to skip to the next 64-bit register, so the code works by accident.  But on an architecture where sizeof(int) < 64-bits and arguments are passed on the stack, it explodes.

     This can cause "fun" 64-bit portability issues.  For example, the following works by accident:

     execl("foo", "foo", NULL);

    This one explodes, because you pass more arguments than the number of integer registers reserved for arguments:

    execl("foo", "foo", "arg1", "arg2", "arg3", "arg4", NULL);

    @pure said:

    I think the world would be a better place if it used PRIx64 from inttypes.h
     

    You get a cookie.  This is the correct answer.


     



  • @Huor said:

    Good lord, there' so much fail in this thread.

    @Ben L. said:

    @flabdablet said:
    D'OH! That printf() format string should be using %p instead of %x

    But they're not pointers. They're unsigned long longs.
     

    Heresy.  From stdint.h on a Red Hat box I'm on:

    # if __WORDSIZE == 64
    typedef long int                int64_t;
    # else
    extension
    typedef long long int           int64_t;
    # endif

    Every post on the internet that purports to correct the errors of others always contains at least one error itself, and this is it. The types in the code sample are uint64.



  •  Oof, which the OP defined himself.  That's what I get for skimming.

     My points about the behaviour of variadic functions stand.



  • @Huor said:

    Completely wrong.  shorts and chars get promoted to int when passed to variadic functions.  ints don't get promoted to anything

    Oops, you are right. I must have confused that with float-to-double promotion in some twisted way. I mostly program in C++ and rarely need variadic functions in my code.



  • Looks like a good example of real-world code with a few minor (and one severe) real-world bugs. I wouldn't expect first-year students to catch the bugs, but for second or third it should be right up their alley.



  • IMHO, the only bug that can be seen with that code is the wrong format string specifier.

    There can be possibly other issues for values returned by functions, but we can't tell that without seeing them.
    For the same reason, "missing free()" doesn't make sense to me. For what i know it could be static memory or part of a bigger dynamic memory block.

    I find the trans->valid a strange design pattern and expected a NULL instead, but it's not a bug.


  • Discourse touched me in a no-no place

    @Zmaster said:

    IMHO, the only bug that can be seen with that code is the wrong format string specifier.
    The first thing that screamed out at me with that code is the future bug waiting to happen 6 months down the line in that switch clause, specifically the abuse of default and the rather presumptuous assumption it's currently making.



  • @HardwareGeek said:


    uint64 addr, data;
    printf("ERROR: Address: %016x Data: %016x %s\n", addr, data, msg);

    That won't compile. At least for anybody sane enough to use -Wall -Werror or at least -Wall -Werror=format with their gcc (or clang).



  • @PJH said:

    @Zmaster said:
    IMHO, the only bug that can be seen with that code is the wrong format string specifier.
    The first thing that screamed out at me with that code is the future bug waiting to happen 6 months down the line in that switch clause, specifically the abuse of default and the rather presumptuous assumption it's currently making.

    How is it abusing default and what do you propose to do instead? Add an explicit case for OTHER and leave any possible future types unhandled? I think it's a perfectly appropriate use of default to report an unexpected value.


  • Discourse touched me in a no-no place

    @tdb said:

    @PJH said:
    @Zmaster said:
    IMHO, the only bug that can be seen with that code is the wrong format string specifier.
    The first thing that screamed out at me with that code is the future bug waiting to happen 6 months down the line in that switch clause, specifically the abuse of default and the rather presumptuous assumption it's currently making.

    How is it abusing default and what do you propose to do instead? Add an explicit case for OTHER and leave any possible future types unhandled? I think it's a perfectly appropriate use of default to report an unexpected value.
    If the enum is expanded, but the switch is not then it will cause, at worst, silent problems, at best, a runtime error. So yes, add an explicit case for every enum, but still have the default clause and scream loudly about an unhandled case (message box, log message whatever.):

            switch (trans->type)
            {
                case READ:
                    doRead(trans);
                    break;
                case WRITE:
                    doWrite(trans);
                    break;
                case OTHER:
                    msg = typeStr(trans->type);
                    printf("ERROR: Address: %016x Data: %016x %s\n",
                            addr, data, msg);
                    break;
                default:
                    syslog(LOG_ERR, "Unhandled switch type %d in %s at %d", trans->type, __FUNCTION__, __LINE__);
                    // exit(-1); depending on how serious it might be.
                    break;
            }
        }
    }
    

    Then again, applying suggestions up-thread about increasing warnings would point this out anyway at compile time.


  • typedef unsigned long long int uint64;

    I'm a C/C++ noob, but this shit... this shit is why I hate those languages with a passion.

    As an aside (and as a noob), what's the purpose of assigning trans->addr and trans->data to temporary variables? Why don't you just reference them directly in printf()?


  • Discourse touched me in a no-no place

    @The_Assimilator said:

    typedef unsigned long long int uint64;

    I'm a C/C++ noob, but this shit... this shit is why I hate those languages with a passion.

    To be honest, they don't really need to be specified in user source files at all, since uint64_t (found in stdint.h) exists as a portable method of specifying a type that has 64 bits (provided your platform can handle them that is.) or uint_least64_t if you need an int that can hold at least 64 bits (other flavours are available.)

    @The_Assimilator said:

    As an aside (and as a noob), what's the purpose of assigning trans->addr and trans->data to temporary variables? Why don't you just reference them directly in printf()?
    Probably a carry-over from the original code where they were referenced frequently - it cuts down on the clutter. Though I agree, the code presented - in isolation - doesn't need to do it.



  • @PJH said:

    @The_Assimilator said:
    typedef unsigned long long int uint64;

    I'm a C/C++ noob, but this shit... this shit is why I hate those languages with a passion.

    To be honest, they don't really need to be specified in user source files at all, since uint64_t (found in stdint.h) exists as a portable method of specifying a type that has 64 bits (provided your platform can handle them that is.) or uint_least64_t if you need an int that can hold at least 64 bits (other flavours are available.)

    Although the (probably familiar) reply to that is that a maintainer can't force the previous people who worked on the code to do things sensibly. Is there a standard compiler flag to warn about use of "raw" int types outside stdint.h?


  • Discourse touched me in a no-no place

    @pjt33 said:

    @PJH said:
    @The_Assimilator said:
    typedef unsigned long long int uint64;

    I'm a C/C++ noob, but this shit... this shit is why I hate those languages with a passion.

    To be honest, they don't really need to be specified in user source files at all, since uint64_t (found in stdint.h) exists as a portable method of specifying a type that has 64 bits (provided your platform can handle them that is.) or uint_least64_t if you need an int that can hold at least 64 bits (other flavours are available.)

    Although the (probably familiar) reply to that is that a maintainer can't force the previous people who worked on the code to do things sensibly. Is there a standard compiler flag to warn about use of "raw" int types outside stdint.h?
    Not that I'm aware of.



    I'm not quite sure how you'd enforce what you're after without generating false-positives on (other standard(-ish)) things such as in_addr_t which aren't in The Standard, but are nevertheless ubiquitous, without special-casing them. And what of in-house derived types; for example in our common headers we have a timestamp_t type which is based on uint64_t.




  • @PJH said:

    I'm not quite sure how you'd enforce what you're after without generating false-positives on (other standard(-ish)) things such as in_addr_t which aren't in The Standard, but are nevertheless ubiquitous, without special-casing them. And what of in-house derived types; for example in our common headers we have a timestamp_t type which is based on uint64_t.

    I haven't explained myself clearly enough. The intention would be to allow use of e.g. uint64_t but to reject direct use of e.g. long int. One of the things that has frustrated me more than once is trying to port something from C and having to deduce from context what width the original author thought an int was.


  • Discourse touched me in a no-no place

    @pjt33 said:

    @PJH said:
    I'm not quite sure how you'd enforce what you're after without generating false-positives on (other standard(-ish)) things such as in_addr_t which aren't in The Standard, but are nevertheless ubiquitous, without special-casing them. And what of in-house derived types; for example in our common headers we have a timestamp_t type which is based on uint64_t.

    I haven't explained myself clearly enough. The intention would be to allow use of e.g. uint64_t but to reject direct use of e.g. long int. One of the things that has frustrated me more than once is trying to port something from C and having to deduce from context what width the original author thought an int was.

    No. There isn't a flag/switch.

    A simple grep through the source might show up a few instances, static analysis tools a few more perhaps. But beyond that I'm unaware of any tools to do what you're after beyond manually slogging through the source.


  • @PJH said:

    @The_Assimilator said:
    As an aside (and as a noob), what's the purpose of assigning trans->addr and trans->data to temporary variables? Why don't you just reference them directly in printf()?
    Probably a carry-over from the original code where they were referenced frequently - it cuts down on the clutter. Though I agree, the code presented - in isolation - doesn't need to do it.
    This is the original code. It's just a sample I threw together quickly to use in the interview. Only the type mismatch between the printf format string and the numeric types was intended to be relevant to the interview; everything else was just camoflage. I intentionally (and in some cases unintentionally) didn't bother with error handling, because it would have provided too much distraction from the intentional bug.

    I did, at one point, use trans->addr and trans->data directly in the the printf(), but my boss thought that was an extra layer of potential confusion, so I assigned them to temporaries to simplify the printf() call.

    I've learned several things from this thread. The target audience for the question is EEs who claim C experience, not CS grads, so this interview question is probably a little too obscure. If I use it again, maybe for more experienced candidates, I will fix some of the points raised by commenters here, but I'll probably leave others to see if candidates pick up on them.

    I learned about a library that, while not even close to new, I had never (that I recall) run across before. That is surprising, because when working directly with the (simulated) hardware, we really care exactly how wide the data is. I guess I have always assumed (because I have never encountered a case where the assumption was not valid) that when I see something like uint64 or uint64_t, whoever defined the type did so in a way that is correct for the compiler we're using. (This is always internal-use-only software, so portability is not usually a major concern. Usually, IT and/or CAD approves specific OS and compiler versions, and we're locked into them for the life of the project.) When I have looked at how they were defined, as far as I can recall, I've found typedefs like I used.

    Finally, I've learned that even when I write quick, one-off stuff like this, I need to be more careful, lest I one day find it on the front page.

     


  • Discourse touched me in a no-no place

    @HardwareGeek said:

    I learned about a library that, while not even close to new, I had never (that I recall) run across before. That is surprising, because when working directly with the (simulated) hardware, we really care exactly how wide the data is. I guess I have always assumed (because I have never encountered a case where the assumption was not valid) that when I see something like uint64 or uint64_t, whoever defined the type did so in a way that is correct for the compiler we're using.
    If you're talking about <stdint.h> it's a header file, not a library...



  • @Zmaster said:

    I find the trans->valid a strange design pattern and expected a NULL instead, but it's not a bug.

    What's really strange about it is that it's only tested as the loop-exit condition, at the bottom of the loop, after the supposedly-invalid entry has been processed by the switch. I would expect an invalid flag to be tested at the start of the loop and used to break out. If the get function sets this flag in the last-valid entry, it should be called no_more_entries or something similar; if the getter returns a dummy entry after the last real one with this flag set, the loop shouldn't process it.



    YMMV, of course.



  • @PJH said:

    If you're talking about <stdint.h> it's a header file, not a library...
    I blame caffeine deficiency.



  • @DaveK said:

    @Zmaster said:

    I find the trans->valid a strange design pattern and expected a NULL instead, but it's not a bug.

    What's really strange about it is that it's only tested as the loop-exit condition, at the bottom of the loop, after the supposedly-invalid entry has been processed by the switch. I would expect an invalid flag to be tested at the start of the loop and used to break out. If the get function sets this flag in the last-valid entry, it should be called no_more_entries or something similar; if the getter returns a dummy entry after the last real one with this flag set, the loop shouldn't process it.

    The condition in a for loop is a precondition, not postcondition. More specifically, this:

    for(a; b; c)
    {
      // ...
    }

    is equivalent to this:

    a;
    while(b)
    {
      // ...
      c;
    }

    (with the minor exception of the scope of declarations in a).



  • @tdb said:

    The condition in a for loop is a precondition, not postcondition.

    Bah, yes, you're right of course. Must have been my turn to screw up! :-)



  • @tdb said:

    @HardwareGeek said:
    Any opinions on whether the difficulty was appropriate? Both candidates got the general cause of the bug, but neither quite nailed it.

    Upon seeing the mention of segfault in printf, the use of 64-bit integers and the %016x format I immediately though of a mismatch between expected and passed parameter size.

    Late to the thread, but as a somewhat fresh CS grad with about a year total of C/C++ experience, this was my first thought as well. I couldn't tell you the correct format to use without looking it up, but I would've checked with you if the one you have is correct. And I would have suggested using uint64_t.


Log in to reply