TDA4VM: Bug reports for TIDL

Ross O'Connor

Part Number: TDA4VM

I have recently had cause to recompile TIDL (tidl_j721e_08_05_00_16, as found with RTOS SDK 08_05_00_11), and have found a number of bugs. I've created patch files which I have attached to shared my fixes. I've also verified that most of these are still extant in the later SDK version (08_06_00_12).

Many of these bugs cause segmentation faults with TIDL is built with modern compilers. Also many generate warnings, but are often suppressed in the Makefiles.

Most of the bugs are due to missing return statements at the end of functions (which with anything more than O1 optimisation will segfault due probably due to leaf function optimisations etc.), although there's also at least one array overrun (both read and write beyond the end of the array).

See attached patches for full details. Please feed these back to the relevant development teams.

Regards,

Ross

over 1 year ago

0 Ross O'Connor over 1 year ago

Prodigy 170 points

tidl_fix_array_overrun_patch.txt

--- a/ti_dl/algo/src/tidl_alg.c	2022-12-20 00:04:28.000000000 +0000
+++ b/ti_dl/algo/src/tidl_alg.c	2023-04-26 08:58:56.775836282 +0100
@@ -188,7 +188,7 @@
 static void TIDL_printMemorySizeStats(const IALG_MemRec memRec[], 
   int32_t numMemRec)
 {
-  int32_t i, totalSize[TIDL_DDR_MEMREC_NON_CACHEABLE] = {0};
+  int32_t i, totalSize[TIDL_DDR_MEMREC_NON_CACHEABLE+1] = {0};
   const char *spaceMapping[TIDL_DDR_MEMREC_NON_CACHEABLE+1];
   const char *attrMapping[IALG_WRITEONCE + 1];
   spaceMapping[IALG_DARAM0]   = "L1D";

tidl_fix_iterators_bug_patch.txt

--- a/ti_dl/utils/tidlModelImport/tidl_import_common.cpp	2022-12-20 00:04:27.000000000 +0000
+++ b/ti_dl/utils/tidlModelImport/tidl_import_common.cpp	2023-04-20 13:43:38.551355435 +0100
@@ -474,8 +474,8 @@
 void TIDL_UpdateInDataNameInLayers(sTIDL_OrgNetwork_t * pOrgTIDLNetStructure,
   uint32_t numLayers, char * currName , char * newName, std::vector<int32_t>& layers)
 {
-  int32_t i, j;
-  for (i : layers)
+  int32_t j;
+  for (int32_t i : layers)
   {
     for (j = 0; j < pOrgTIDLNetStructure->TIDLPCLayers[i].numInBufs; j++)
     {
@@ -950,8 +950,8 @@
   uint32_t numLayers, sTIDL_DataParams_t dataBuf,  sTIDL_DataParams_t newDataBuf,
   std::vector<int32_t>& layers)
 {
-  int32_t i, j;
-  for (i : layers)
+  int32_t j;
+  for (int32_t i : layers)
   {
     for (j = 0; (j < pOrgTIDLNetStructure->TIDLPCLayers[i].numInBufs) &&
       (pOrgTIDLNetStructure->TIDLPCLayers[i].numInBufs > 0); j++)
@@ -8634,7 +8634,6 @@
 int32_t tidl_addUnStitchLayerForBatchProcessing(sTIDL_OrgNetwork_t  *pOrgTIDLNetStructure, int32_t * dataIndex,
         int32_t layerIndex, int32_t i1, std::vector<int32_t>& out_layers, int32_t *layerID)
 {
-  int32_t i2;
   char dataLayerName[10];
   pOrgTIDLNetStructure->TIDLPCLayers[layerIndex].layerType = TIDL_BatchReshapeLayer;
   pOrgTIDLNetStructure->TIDLPCLayers[layerIndex].numInBufs  = 1; //pOrgTIDLNetStructure->TIDLPCLayers[i1].numOutBufs;
@@ -8645,7 +8644,7 @@
   strcpy((char *)pOrgTIDLNetStructure->TIDLPCLayers[layerIndex].outDataNames[0], (char *)pOrgTIDLNetStructure->TIDLPCLayers[i1].outDataNames[0]);
   strcat((char *)pOrgTIDLNetStructure->TIDLPCLayers[i1].outDataNames[0], "_batchFormat");
   strcpy((char *)pOrgTIDLNetStructure->TIDLPCLayers[layerIndex].inDataNames[0], (char *)pOrgTIDLNetStructure->TIDLPCLayers[i1].outDataNames[0]);
-  for (i2 : out_layers)
+  for (int32_t i2 : out_layers)
   {
     strcpy((char *)pOrgTIDLNetStructure->TIDLPCLayers[i2].inDataNames[0], (char *)pOrgTIDLNetStructure->TIDLPCLayers[layerIndex].outDataNames[0]);
   }
@@ -8719,7 +8718,7 @@
 */
 int32_t tidl_addBatchReshapeLayerForBatchProcessing(sTIDL_OrgNetwork_t  &pOrgTIDLNetStructure, int32_t * dataIndex, int32_t layerIndex, int32_t *layerID, int32_t *batchPadTotal)
 {
-  int32_t i1, i2, i3, i4;
+  int32_t i1, i2, i4;
   char dataLayerName[10];
   int32_t status = 0;
   int32_t lyrAdded = 0;
@@ -8738,7 +8737,7 @@
         {
           std::vector<int32_t> out_layers = tidl_getOutLayers(pOrgTIDLNetStructure, layerIndex, pOrgTIDLNetStructure.TIDLPCLayers[i1].inData[i2].dataId);
           std::vector<int32_t> out_layers_support_stitching;
-          for (i3 : out_layers)
+          for (int32_t i3 : out_layers)
           {
             if (TIDL_doesLayerSupportBatchProcessing(&pOrgTIDLNetStructure.TIDLPCLayers[i3]) == 1)
             {
@@ -8781,7 +8780,7 @@
         if (out_layers.size() == 0)
           return -1;
         std::vector<int32_t> out_layers_donot_support_stitching;
-        for (i3 : out_layers)
+        for (int32_t i3 : out_layers)
         {
           sTIDL_LayerPC_t &TIDLPCOutLayers = pOrgTIDLNetStructure.TIDLPCLayers[i3];
           if ((TIDLPCOutLayers.layerType != TIDL_BatchReshapeLayer) &&

tisci_fix_variable_array_bug_patch.txt

--- a/packages/ti/drv/sciclient/soc/sysfw/include/tisci/tisci_protocol.h	2022-12-20 00:04:10.000000000 +0000
+++ b/packages/ti/drv/sciclient/soc/sysfw/include/tisci/tisci_protocol.h	2023-04-21 09:45:47.863739477 +0100
@@ -92,11 +92,7 @@
     uint8_t    seq;
     uint32_t    flags;
     /* Windows Visual Studio build has issues with  payload[], changing it only for visual studio build */ 
-#ifdef _MSC_VER 
     uint8_t    payload; 
-#else 
-    uint8_t    payload[]; 
-#endif
 };
 
 /*

tidl_fix_no_return_bugs_patch.txt

--- a/arm-tidl/onnxrt_ep/src/concerto_common.mak	2022-12-20 00:04:22.000000000 +0000
+++ b/arm-tidl/onnxrt_ep/src/concerto_common.mak	2023-04-24 10:50:28.586491914 +0100
@@ -26,4 +26,5 @@
              -Wno-unused-but-set-variable \
              -Wno-unused-result \
              -Wno-format-overflow \
-             -Wno-format-truncation
+             -Wno-format-truncation \
+             -Werror=return-type
--- a/arm-tidl/rt/src/concerto_common.mak	2022-12-20 00:04:21.000000000 +0000
+++ b/arm-tidl/rt/src/concerto_common.mak	2023-04-24 10:50:16.170516566 +0100
@@ -1,6 +1,6 @@
 TARGET      := vx_tidl_rt
 TARGETTYPE  := dsmo
-CFLAGS += -fPIC -Wno-int-to-pointer-cast -Wno-stringop-truncation -Wno-format-overflow
+CFLAGS += -fPIC -Wno-int-to-pointer-cast -Wno-stringop-truncation -Wno-format-overflow -Werror=return-type
 CPPFLAGS += -fPIC --std=c++11
 
 CSOURCES    += ../tidl_rt_ovx.c
--- a/makerules/rules.mk	2022-12-20 00:04:21.000000000 +0000
+++ b/makerules/rules.mk	2023-04-24 13:04:16.778657781 +0100
@@ -224,13 +224,13 @@
   endif
 
   ifeq ($(TARGET_BUILD), debug)
-    COMPILER_FLAGS += -std=c++14  -DHOST_EMULATION -w -D_HOST_BUILD -DGCC_BUILD  
+    COMPILER_FLAGS += -std=c++14  -DHOST_EMULATION -D_HOST_BUILD -DGCC_BUILD -Werror=return-type
     GCC_DEBUG_CFLAGS :=-ggdb -ggdb3 -gdwarf-2
   else
     ifeq ($(DEVELOPER_BUILD), 0)
-      COMPILER_FLAGS += -std=c++14 -O3 -DHOST_EMULATION -D_HOST_BUILD -DGCC_BUILD 
+      COMPILER_FLAGS += -std=c++14 -O3 -DHOST_EMULATION -D_HOST_BUILD -DGCC_BUILD -Werror=return-type
     else
-      COMPILER_FLAGS += -std=c++14 -O3 -DHOST_EMULATION -D_HOST_BUILD -DGCC_BUILD -g
+      COMPILER_FLAGS += -std=c++14 -O3 -DHOST_EMULATION -D_HOST_BUILD -DGCC_BUILD -g -Werror=return-type
     endif
   endif
   LD=g++-5
Binary files a/ti_dl/test/j7-c71_0-fw and b/ti_dl/test/j7-c71_0-fw differ
--- a/ti_dl/utils/tidlModelGraphviz/tidl_graph_model_runtimes.cpp	2022-12-20 00:04:27.000000000 +0000
+++ b/ti_dl/utils/tidlModelGraphviz/tidl_graph_model_runtimes.cpp	2023-04-24 08:30:03.523153079 +0100
@@ -56,7 +56,7 @@
   "#EE82EE",    //"Scale"
 };
 
-void * TIDL_readAllowlistNodesFile(char * fname, std::vector<std::vector<int>>& supportedNodeGroups)
+void TIDL_readAllowlistNodesFile(char * fname, std::vector<std::vector<int>>& supportedNodeGroups)
 {
   std::ifstream fAllowlistNode(fname);
 
--- a/ti_dl/utils/tidlModelImport/makefile_bin	2022-12-20 00:04:27.000000000 +0000
+++ b/ti_dl/utils/tidlModelImport/makefile_bin	2023-04-24 12:30:43.494549119 +0100
@@ -98,7 +98,7 @@
 CFLAGS += /DTIDL_IMPORT_TOOL
 else
 CFLAGS += -D_GNU_SOURCE
-CFLAGS += -DTIDL_IMPORT_TOOL -w
+CFLAGS += -DTIDL_IMPORT_TOOL -Werror=return-type
 endif
 
 ##############################################################
--- a/ti_dl/utils/tidlModelImport/makefile_lib	2022-12-20 00:04:27.000000000 +0000
+++ b/ti_dl/utils/tidlModelImport/makefile_lib	2023-04-24 12:30:50.454535298 +0100
@@ -116,7 +116,7 @@
 ifdef SystemRoot
 CFLAGS+= /DTIDL_IMPORT_TOOL
 else
-CFLAGS+= -DTIDL_IMPORT_TOOL -w
+CFLAGS+= -DTIDL_IMPORT_TOOL -Werror=return-type
 endif
 
 ##############################################################
--- a/ti_dl/utils/tidlModelImport/makefile_shared	2022-12-20 00:04:27.000000000 +0000
+++ b/ti_dl/utils/tidlModelImport/makefile_shared	2023-04-24 13:40:36.814848128 +0100
@@ -181,7 +181,7 @@
 CFLAGS += /DDLL_EXPORT
 else
 CFLAGS += -D_GNU_SOURCE
-CFLAGS += -DTIDL_IMPORT_TOOL -w
+CFLAGS += -DTIDL_IMPORT_TOOL -Werror=return-type
 endif
 
 ifeq ($(MAKECMDGOALS),tvm_relay)
--- a/ti_dl/utils/tidlModelImport/tidl_import_common.cpp	2023-04-24 13:38:45.283719711 +0100
+++ b/ti_dl/utils/tidlModelImport/tidl_import_common.cpp	2023-04-24 12:10:00.353017409 +0100
@@ -922,6 +922,8 @@
   {
     fclose(fp1);
   }
+
+  return 0;
 }
 
 void TIDL_UpdateInDataBuffId(sTIDL_OrgNetwork_t * pOrgTIDLNetStructure,
@@ -1650,6 +1652,7 @@
 
   fwrite(&gIOParams, 1, sizeof(sTIDL_IOBufDesc_t), fp1);
   fclose(fp1);
+  return 0;
 }
 
 
--- a/ti_dl/utils/tidlModelImport/tidl_import_common_model_check.cpp	2022-12-20 00:04:27.000000000 +0000
+++ b/ti_dl/utils/tidlModelImport/tidl_import_common_model_check.cpp	2023-04-24 11:00:02.057353286 +0100
@@ -461,6 +461,8 @@
       return TIDL_ALLOWLISTING_LAYER_CHECK_FAILED;
     }
   }
+
+  return 0;
 }
 static void checkConvLayers(const sTIDL_LayerPC_t &layerPC, DiagList_t &diags)
 {
--- a/ti_dl/utils/tidlModelImport/tidl_import_core.cpp	2022-12-20 00:04:27.000000000 +0000
+++ b/ti_dl/utils/tidlModelImport/tidl_import_core.cpp	2023-04-24 12:09:15.497106472 +0100
@@ -558,7 +558,7 @@
   {
     printf("%s\n", sysCommand);
   }
-  system(sysCommand);
+  return system(sysCommand);
 }
 
 int tidlRunModelDumpTool(tidl_import_config * params)
@@ -586,7 +586,7 @@
   {
     printf("%s\n", sysCommand);
   }
-  system(sysCommand);
+  return system(sysCommand);
 }
 
 int tidlWriteTensorNamesToFile(tidl_import_config * params, const char* suffix)
@@ -623,6 +623,7 @@
   }
   free(tidlNet);
   fclose(layerInfoFile);
+  return 0;
 }
 
 /** This function removes the string "tidl_net_" and ".bin" from the net bin file names to reduce the folder name size for perfsim
@@ -1198,6 +1199,8 @@
   {
     pTIDLNetStructure->isQuantStatsAvailable = quantStatus;
   }
+
+  return 0;
 }
 
 void tidl_updateNetPitch(sTIDL_Network_t * tidlNet)
--- a/ti_dl/utils/tidlModelImport/tidl_import_quantize.cpp	2022-12-20 00:04:27.000000000 +0000
+++ b/ti_dl/utils/tidlModelImport/tidl_import_quantize.cpp	2023-04-24 12:14:34.552472971 +0100
@@ -2341,6 +2341,7 @@
     /* Run quant stats again to save the final min/max statistics after bias calibration */
 //    tidlRunQuantStatsTool((void**)&perChannelMeanPtrFloat);
 
+  return 0;
 }
 
 
--- a/ti_dl/utils/tidlModelImport/tidl_runtimes_import_common.cpp	2022-12-20 00:04:27.000000000 +0000
+++ b/ti_dl/utils/tidlModelImport/tidl_runtimes_import_common.cpp	2023-04-24 12:37:52.093698124 +0100
@@ -710,6 +710,8 @@
     printf("Please provide TIDL tools path \n");
     exit(-1);
   }
+
+  return 0;
 }
 
 /** Add output data layer to subgraph */
@@ -785,6 +787,8 @@
       gParams.rawDataInElementType[i] = TIDL_SignedWord;
     }
   }
+
+  return 0;
 }
 
 /** This function calls "TIDL_import_backend" to run calibration, quantization and save the final network files */
--- a/ti_dl/utils/tidlModelImport/tidl_tfImport.cpp	2022-12-20 00:04:27.000000000 +0000
+++ b/ti_dl/utils/tidlModelImport/tidl_tfImport.cpp	2023-04-24 12:10:32.700953181 +0100
@@ -1259,6 +1259,8 @@
       strcat(outList, ",");
    }
   }
+
+  return 0;
 }
 
 int32_t tidl_tfLayerFillTensorNames(sTIDL_OrgNetwork_t   *pOrgTIDLNetStructure,
--- a/ti_dl/utils/tidlModelImport/tidl_tfLiteImport.cpp	2022-12-20 00:04:27.000000000 +0000
+++ b/ti_dl/utils/tidlModelImport/tidl_tfLiteImport.cpp	2023-04-24 12:11:21.556856176 +0100
@@ -322,6 +322,7 @@
     printf("\nOnly float, DT_INT32 and DT_UNT8 tensor is suported \n");
     return -1;
   }
+  return 0;
 }
 
 bool isTensorVariable(const Model* tfliteModel,int32_t idx)
@@ -1333,6 +1334,7 @@
     return -1;
   }
 
+  return 0;
 }
 
 int32_t TIDL_tfliteMapReluParams(sTIDL_OrgNetwork_t   *pOrgTIDLNetStructure,
--- a/ti_dl/test/src/concerto_common.mak	2022-12-20 00:04:28.000000000 +0000
+++ b/ti_dl/test/src/concerto_common.mak	2023-04-25 12:41:17.620684302 +0100
@@ -51,6 +51,10 @@
     DEFS+=DMA_UTILS_STANDALONE
 endif
 
+ifeq ($(TARGET_PLATFORM),PC)
+    CFLAGS += -Werror=return-type
+endif
+
 #ifeq ($(TIDL_BUILD_FOR_QT), 1)
 DEFS+=TIDL_BUILD_FOR_QT
 #endif
--- a/ti_dl/test/src/tidl_rt.c	2022-12-20 00:04:28.000000000 +0000
+++ b/ti_dl/test/src/tidl_rt.c	2023-04-25 12:42:50.396648161 +0100
@@ -256,13 +256,14 @@
   return;
 }
 
-tidl_printDdrStats(uint64_t read_bytes, uint64_t write_bytes)
+int tidl_printDdrStats(uint64_t read_bytes, uint64_t write_bytes)
 {
 #if (!HOST_EMULATION)
   #if ENABLE_DDR_BW_STATS
       tidl_tb_printf(0, " DDR_READ_WRITE_TOTAL_MB = %8.2f, %8.2f, %8.2f ", (ddr_read / 1000000.0), (ddr_write / 1000000.0), ((ddr_read + ddr_write) / 1000000.0));
   #endif
 #endif
+  return 0;
 } 
 
 #if (!HOST_EMULATION)

Re-attaching files as it seems to have ignored .patch extension, so renamed as _patch.txt instead.

0 Stanley Liu over 1 year ago in reply to Ross O'Connor

TI__Guru 50045 points

Hi Ross,

Thanks for sharing the patches. Which compiler did you use?

Regards,
Stanley

0 Ross O'Connor over 1 year ago in reply to Stanley Liu

Prodigy 170 points

Hi Stanley,

I am using gcc 11 (where your Makefiles want gcc-5).

Due to the nature of my employer, I work on a secure network with no outside internet access, so we cannot get access to the old versions of OS and tools your Makefiles want (given they have known security issues).

However, I would point out that these are all real bugs and I'd certainly advise against relying on old compiler versions getting away with a safer undefined behaviour than others :)

Thanks,

Ross

+1 Pratik Kedar over 1 year ago in reply to Ross O'Connor

TI__Mastermind 24021 points

Hi Ross,

As I can see, the issues you have elaborated in patch 1(tidl_fix_array_overrun_patch.txt) and patch 2(tidl_fix_iterators_bug_patch.txt), are fixed in SDK 8.6.

Regarding patch tisci_fix_variable_array_bug_patch.txt i have created internal request for it.

Adding jira link for TI internal tracking purpose here : https://jira.itg.ti.com/browse/PDK-12850

Regarding patch tidl_fix_no_return_bugs_patch.txt I have created internal jira request for it.

Adding jira link for TI internal tracking purpose here : https://jira.itg.ti.com/browse/TIDL-3331

Regards,

Pratik

0 Ross O'Connor over 1 year ago in reply to Pratik Kedar

Prodigy 170 points

That's great, thanks Pratik.

I assume those links are internal TI servers (I get DNS error following them).

Ross

0 Ross O'Connor over 1 year ago in reply to Ross O'Connor

Prodigy 170 points

Quick note to say I had to change tisci_fix_variable_array_bug_patch.txt to remove the variable length field completely (instead of change it to length one), as the original patch broke target initialisation. Seems like nothing references this field anyway, so it was fine to straight up remove it (and sizeof returns zero for variable length arrays in structs).

0 Stanley Liu over 1 year ago in reply to Ross O'Connor

TI__Guru 50045 points

Thanks for the feedback.

Processors

Processors forum

TDA4VM: Bug reports for TIDL