Adding new cameras, applying patches into trunk (with source code prepared)

  • 1473 Replies
  • 170390 Views
  • Publish
    Advertisements
    Look at the lines I've highlighted - GCC 4.4.0 with 'de' uninitialized is passing and testing the address 12 bytes past the end of the 'de' memory.
    This shows nothing, because variable in literal pool have different value, and we dont see here address of "de" in memory.
    The generated code may be correct in both cases.
    The only thing that is changed when you initialize "de" is that it's moved from .bss to .data section.
    The wrong code is at next two lines:
    Code: (c) [Select]
     _ReadFastDir(d, &de);
      return de[0]? &de : (void*)0;
    "de" itself is an address of array, by taking address of it you get pointer to pointer.
    GCC silently casts pointer to pointer type to just pointer.
    I don't believe that this can lead to some problems, but this is definitely wrong.
    « Last Edit: 16 / April / 2011, 06:50:04 by cppasm »

    *

    Online philmoz

    • *****
    • 2936
      • Photos
  • Publish
    Look at the lines I've highlighted - GCC 4.4.0 with 'de' uninitialized is passing and testing the address 12 bytes past the end of the 'de' memory.
    This shows nothing, because variable in literal pool have different value, and we dont see here address of "de" in memory.
    The generated code may be correct in both cases.
    The only thing that is changed when you initialize "de" is that it's moved from .bss to .data section.

    If that were the only thing that changed then why does the compiler generate different code?
    Why does the original code generated by GCC 3.4.6 & GCC 4.5.1 work; but the code generated by GCC 4.4.0 does not - if this is not a compiler bug what is it?
    Look closely at the original code generated by GCC 4.4.0 - it is passing R4+52 as the address to _ReadFastDir and is then checking the value at R4+52; but it returns R4. This is clearly wrong, the code when generated correctly uses the same address for all three uses of 'de', when passed to _ReadFastDir, when tested after that call and as the return value of the function.
    Using two different addresses is a compiler bug - it is generating bad code pure and simple.

    Quote
    The wrong code is at next two lines:
    Code: (c) [Select]
      _ReadFastDir(d, &de);
      return de[0]? &de : (void*)0;
    "de" itself is an address of array, by taking address of it you get pointer to pointer.
    GCC silently casts pointer to pointer type to just pointer.
    I don't believe that this can lead to some problems, but this is definitely wrong.

    This is an incorrect; but common mistake, arising from a misunderstanding of the difference between arrays and pointers in C.
    An array name in C is not a pointer, it is the address of the first element of the array.
    Using the example from readdir, then in C the following are all true:
      de == &de, &de == &de[0] and de == &de[0]
    all forms return the same physical address.
    Technically they are slightly different 'de' and '&de[0]' are the address of the first element, while '&de' is the address of the entire array. Practically this makes no difference in the readdir code - the code is perfectly valid C; although perhaps not the most readable form.

    Phil.
    CHDK ports:
      sx30is (1.00c, 1.00h, 1.00l, 1.00n & 1.00p)
      g12 (1.00c, 1.00e, 1.00f & 1.00g)
      sx130is (1.01d & 1.01f)
      ixus310hs (1.00a & 1.01a)
      sx40hs (1.00d, 1.00g & 1.00i)
      g1x (1.00e, 1.00f & 1.00g)

  • Publish
    Look closely at the original code generated by GCC 4.4.0 - it is passing R4+52 as the address to _ReadFastDir and is then checking the value at R4+52; but it returns R4.
    Yes, that is true. This looks like compiler bug.
    Does it persists with the latest released gcc 4.4.6?
    This is an incorrect; but common mistake, arising from a misunderstanding of the difference between arrays and pointers in C.
    An array name in C is not a pointer, it is the address of the first element of the array.
    I understand the difference.
    Name of the array is not a pointer (pointer itself have a size, while array name does not), but casts to it.
    As I wrote - this is not an error, but taking address of "de" is not neccessary here, and is wrong.

    Here is a testcase:
    Code: (c) [Select]
    char *test_fn(void)
    {
        static char test[40];
        return &test;
    }

    GCC 4.6.0:
    arm-elf-gcc -c -O2 -nostdlib -Wall test.c
    test.c: In function 'test_fn':
    test.c:4:5: warning: return from incompatible pointer type [enabled by default]

    VS 2005:
    cl /c /Ox /W3 test.c
    Microsoft (R) C/C++ Optimizing Compiler Version 14.00.50727.762 for x64
    Copyright (C) Microsoft Corporation.  All rights reserved.

    test.c
    test.c(4) : warning C4047: 'return' : 'char *' differs in levels of indirection from 'char (*)[40]'

    Changing code to "return test;" resolves all this warnings.
    « Last Edit: 16 / April / 2011, 12:04:31 by cppasm »

    *

    Online philmoz

    • *****
    • 2936
      • Photos
  • Publish
    Changing code to "return test;" resolves all this warnings.

    Add 'printf("%08x %08x %08x\n",test,&test,&test[0]);' before the return and run the resulting programs.
    You should get the same memory address printed 3 times.

    As I said technically they are different things, which is what the compiler is warning you about.
    But the end result is identical compiled code so it makes no difference when it is running.

    Returning the address of 'de' in readdir may not be necessary or good practice; but it is not wrong.

    The return type for readdir in wrappers.c is void*, whereas the definition in stdlib.h is 'struct dirent *' - that's a far worse coding practice and abuse of the C language than returning '&de'.

    Phil.
    CHDK ports:
      sx30is (1.00c, 1.00h, 1.00l, 1.00n & 1.00p)
      g12 (1.00c, 1.00e, 1.00f & 1.00g)
      sx130is (1.01d & 1.01f)
      ixus310hs (1.00a & 1.01a)
      sx40hs (1.00d, 1.00g & 1.00i)
      g1x (1.00e, 1.00f & 1.00g)


    *

    Online philmoz

    • *****
    • 2936
      • Photos
  • Publish
    G12 patch to cleanup boot.c task hook (removes changes made while tracking down startup crash that are not needed).
    Also adds display of exmem start address when OPT_EXMEM_TESTING is enabled. This is the value to use for MEMISOSTART when using OPT_CHDK_IN_EXMEM (saves calculating it manually).

    Phil.
    CHDK ports:
      sx30is (1.00c, 1.00h, 1.00l, 1.00n & 1.00p)
      g12 (1.00c, 1.00e, 1.00f & 1.00g)
      sx130is (1.01d & 1.01f)
      ixus310hs (1.00a & 1.01a)
      sx40hs (1.00d, 1.00g & 1.00i)
      g1x (1.00e, 1.00f & 1.00g)

  • Publish
    Attached please find a patch file to add support for IXUS120-SD940 firmware version 1.01A.   Thanks to forum members gesualdo and zawin for their testing work.

  • Publish
    A small patch to the value used for  vid_get_viewport_width()  in IXUS120-SD940 firmware 1.02C that aligns it with the other three firmware versions of this camera.

    *

    Offline reyalp

    • ******
    • 9951
  • Publish
    G12 patch to cleanup boot.c task hook (removes changes made while tracking down startup crash that are not needed).
    Also adds display of exmem start address when OPT_EXMEM_TESTING is enabled. This is the value to use for MEMISOSTART when using OPT_CHDK_IN_EXMEM (saves calculating it manually).
    Added changeset 1148

    A small patch to the value used for  vid_get_viewport_width()  in IXUS120-SD940 firmware 1.02C that aligns it with the other three firmware versions of this camera.
    Added changeset 1149

    A small patch to the value used for  vid_get_viewport_width()  in IXUS120-SD940 firmware 1.02C that aligns it with the other three firmware versions of this camera.
    Added changeset 1150
    « Last Edit: 17 / April / 2011, 15:36:20 by reyalp »
    Don't forget what the H stands for.



    *

    Online philmoz

    • *****
    • 2936
      • Photos
  • Publish
    Patch for G12 & SX30 (patched against changeset 1150).

    While looking at the EXMEM code in the firmware I noticed the 'memshow' eventproc that displays various useful information about the firmware heap memory allocation.

    Tracing through this code I found a function that fills an array of 10 int's with this memory info. It gives things like start and end address of the heap, total size, size allocated, free space and size of the largest free block. This last one is what the code in 'core_get_free_memory' (in main.c) attempts to calculate in a somewhat crude manner.

    It would seem that calling the firmware function would be a good replacement for the current 'core_get_free_memory' code.

    I've called this function GetMemInfo in the G12 & SX30 patch.

    This patch adds:
    - new define in camera.h CAM_FIRMWARE_MEMINFO (include comments on how to find the above function).
    - prototype for GetMemInfo and struct definition of the data returned in platform.h
    - change to core_get_free_memory to use GetMemInfo if CAM_FIRMWARE_MEMINFO is defined in main.c
    - added wrapper for the firmware function in generic/wrappers.c
    - added stubs for G12 and SX30 for GetMemInfo (surprisingly it's the same address for both cameras across all firmware versions).

    I checked the firmware for the SX20, D10, S95 and IXUS1000 and they all appear to have this function.

    Phil.

    Edit: patch removed.

    « Last Edit: 02 / May / 2011, 07:30:32 by philmoz »
    CHDK ports:
      sx30is (1.00c, 1.00h, 1.00l, 1.00n & 1.00p)
      g12 (1.00c, 1.00e, 1.00f & 1.00g)
      sx130is (1.01d & 1.01f)
      ixus310hs (1.00a & 1.01a)
      sx40hs (1.00d, 1.00g & 1.00i)
      g1x (1.00e, 1.00f & 1.00g)

     

    Related Topics