Hadoop Map/Reduce
  1. Hadoop Map/Reduce
  2. MAPREDUCE-1166

SerialUtils.cc: dynamic allocation of arrays based on runtime variable is not portable

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      In SerialUtils.cc, the following code appears:

      int len;
      if (b < -120)

      { negative = true; len = -120 - b; }

      else

      { negative = false; len = -112 - b; }

      uint8_t barr[len];

      as far as I'm aware, this is not legal in ANSI C and will be rejected by ANSI compliant compilers. Instead, this should be malloc()'d based upon the size of len and free()'d later.

      1. MAPREREDUCE-1166.patch
        0.5 kB
        Allen Wittenauer

        Activity

        Allen Wittenauer created issue -
        Hide
        Brian Bockelman added a comment -

        This is legal in C99, not C89. Depends on which one you call ANSI C (I think it usually refers to C89)

        I believe it's better programming practice to allocate on the heap with malloc/free; I'm not sure if you gain any speed advantages either way.

        Show
        Brian Bockelman added a comment - This is legal in C99, not C89. Depends on which one you call ANSI C (I think it usually refers to C89) I believe it's better programming practice to allocate on the heap with malloc/free; I'm not sure if you gain any speed advantages either way.
        Hide
        Allen Wittenauer added a comment -

        heh. I went with:

        uint8_t barr=(uint8_t * ) alloca(sizeof(uint8_t)(len));

        as my local fix, which SunStudio seems to accept as valid at least.

        Show
        Allen Wittenauer added a comment - heh. I went with: uint8_t barr=(uint8_t * ) alloca(sizeof(uint8_t) (len)); as my local fix, which SunStudio seems to accept as valid at least.
        Allen Wittenauer made changes -
        Field Original Value New Value
        Project Hadoop Common [ 12310240 ] Hadoop Map/Reduce [ 12310941 ]
        Key HADOOP-6263 MAPREDUCE-1166
        Hide
        Allen Wittenauer added a comment -

        Changes barr to use alloca

        Show
        Allen Wittenauer added a comment - Changes barr to use alloca
        Allen Wittenauer made changes -
        Attachment MAPREREDUCE-1166.patch [ 12423600 ]
        Allen Wittenauer made changes -
        Attachment MAPREREDUCE-1166.patch [ 12423600 ]
        Allen Wittenauer made changes -
        Attachment MAPREREDUCE-1166.patch [ 12423604 ]
        Allen Wittenauer made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12423604/MAPREREDUCE-1166.patch
        against trunk revision 831037.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/108/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/108/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/108/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/108/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12423604/MAPREREDUCE-1166.patch against trunk revision 831037. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/108/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/108/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/108/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/108/console This message is automatically generated.
        Chris Douglas made changes -
        Assignee Allen Wittenauer [ aw ]
        Hide
        Eli Collins added a comment -

        Hey Allen,

        Does this code not compile on solaris? Perhaps the build is just not passing -xc99.

        From quickly looking at the code it looks like len will never be greater than 8 in which case an appropriate assert/check could be added and the allocation could read:

        uint8_t barr[sizeof(int64_t)];
        

        The disadvantages of alloca (http://www.gnu.org/s/libc/manual/html_node/Disadvantages-of-Alloca.html) don't apply here but it doesn't seem necessary either.

        Thanks,
        Eli

        Show
        Eli Collins added a comment - Hey Allen, Does this code not compile on solaris? Perhaps the build is just not passing -xc99. From quickly looking at the code it looks like len will never be greater than 8 in which case an appropriate assert/check could be added and the allocation could read: uint8_t barr[sizeof(int64_t)]; The disadvantages of alloca ( http://www.gnu.org/s/libc/manual/html_node/Disadvantages-of-Alloca.html ) don't apply here but it doesn't seem necessary either. Thanks, Eli
        Hide
        Allen Wittenauer added a comment -

        I don't remember if I forced -xc99 or not. I suspect I didn't or I didn't use -xc99=all. If I get a chance I'll try it again. Although it'd be great to avoid C99 to give us greater compatibility. Given other issues (_func_), it is probably unavoidable though.

        Show
        Allen Wittenauer added a comment - I don't remember if I forced -xc99 or not. I suspect I didn't or I didn't use -xc99=all. If I get a chance I'll try it again. Although it'd be great to avoid C99 to give us greater compatibility. Given other issues (_ func _), it is probably unavoidable though.
        Hide
        Allen Wittenauer added a comment -

        So I spent some time looking at this today and trying to get this to work with VLAs in SunStudio. Doing research, I ran across this:

        http://forums.sun.com/thread.jspa?threadID=5348093

        "C99 VLAs were discussed in the C++ Committee for possible inclusion in the next standard. The unanimous view was that they did not have nice properties (the word "suck" was bandied about), and that std::vector or other containers were a superior solution in C++ programming."

        Thus, while _func_ works with CC -features=extensions, it doesn't appear that Sun Studio supports all of the C99 features in their C++ compiler. This makes me wonder if using _func_ was the right thing to do now.

        They also have an interesting discussion about alloca vs. malloc.

        Show
        Allen Wittenauer added a comment - So I spent some time looking at this today and trying to get this to work with VLAs in SunStudio. Doing research, I ran across this: http://forums.sun.com/thread.jspa?threadID=5348093 "C99 VLAs were discussed in the C++ Committee for possible inclusion in the next standard. The unanimous view was that they did not have nice properties (the word "suck" was bandied about), and that std::vector or other containers were a superior solution in C++ programming." Thus, while _ func _ works with CC -features=extensions, it doesn't appear that Sun Studio supports all of the C99 features in their C++ compiler. This makes me wonder if using _ func _ was the right thing to do now. They also have an interesting discussion about alloca vs. malloc.
        Hide
        Allen Wittenauer added a comment -

        This patch isn't quite complete.

        Show
        Allen Wittenauer added a comment - This patch isn't quite complete.
        Allen Wittenauer made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Hide
        Allen Wittenauer added a comment -

        Hadoop's focus is not on portability. Closing as won't fix.

        Show
        Allen Wittenauer added a comment - Hadoop's focus is not on portability. Closing as won't fix.
        Allen Wittenauer made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Won't Fix [ 2 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Patch Available Patch Available
        42d 20h 1m 1 Allen Wittenauer 29/Oct/09 18:38
        Patch Available Patch Available Open Open
        68d 7h 2m 1 Allen Wittenauer 06/Jan/10 01:40
        Open Open Resolved Resolved
        665d 16h 7m 1 Allen Wittenauer 02/Nov/11 17:48

          People

          • Assignee:
            Allen Wittenauer
            Reporter:
            Allen Wittenauer
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development