Merge pull request #1579 from DataDog/nsavoire/fix_elf_dynamic_symbols

Fix upload of ELF dynamic symbols

Author: nsavoire

src/commands/elf-symbols/README.md

@@ -50,4 +50,4 @@ If location is a file, command will split debug info from it and upload them to
 | `--disable-git`    | Optional | Prevents the command from invoking Git in the current working directory and sending repository-related data to Datadog (such as the hash, remote URL, and paths within the repository of sources referenced in the source map). |
 | `--repository-url` | Optional | Overrides the remote repository with a custom URL. For example, `https://github.com/my-company/my-project`. |
 | `--replace-existing` | Optional | If symbol information with the same build ID is already present on Datadog side, discard it and use the newly uploaded information.<br>Default behavior is to only replace existing debug information if the newly uploaded information is considered a better source with the following ordering: debug info > symbol table > dynamic symbol table. |
-| `--upload-dynamic-symbols` | Optional | Upload dynamic symbol information if neither debug information nor symbol table are present but a dynamic symbol table is available.<br>Default behavior is to upload symbol information only when a symbol table or debug information are present, since dynamic symbol table as less information and only contains exported symbols.  |
+| `--upload-dynamic-symbols` | Optional | Upload dynamic symbol information if neither debug information nor symbol table are present but a dynamic symbol table is available.<br>Default behavior is to upload symbol information only when a symbol table or debug information are present, since dynamic symbol table has less information and only contains exported symbols.  |

src/commands/elf-symbols/__tests__/elf.test.ts

@@ -736,7 +736,7 @@ describe('elf', () => {
         goBuildId: '',
         elfType: 'DYN',
         hasDebugInfo: true,
-        hasDynamicSymbolTable: true,
+        hasDynamicSymbolTable: false,
         hasSymbolTable: true,
         hasCode: false,
       })
@@ -831,15 +831,19 @@ describe('elf', () => {
       const filename = path.basename(elfFile)
       const outputFilename = `${tmpDirectory}/${filename}.debug`
       const elfFileMetadata = await getElfFileMetadata(elfFile)
-      await copyElfDebugInfo(elfFile, outputFilename, elfFileMetadata, false)
+      await copyElfDebugInfo(elfFile, outputFilename, elfFileMetadata, true)
       const debugInfoMetadata = await getElfFileMetadata(outputFilename)
 
       // check that elf and debug info metadata are equal except for hasCode and filename
+      // dynamic symbol table is kept only if there is no debug info nor symbol table
+      const hasDynamicSymbolTable =
+        !elfFileMetadata.hasDebugInfo && !elfFileMetadata.hasSymbolTable && elfFileMetadata.hasDynamicSymbolTable
       expect(debugInfoMetadata).toEqual({
         ...elfFileMetadata,
         hasCode: false,
         filename: outputFilename,
         fileHash: debugInfoMetadata.fileHash,
+        hasDynamicSymbolTable,
       })
     }
 

src/commands/elf-symbols/__tests__/fixtures/dyn_x86_64

@@ -114,7 +114,6 @@ describe('elf-symbols upload', () => {
         `${fixtureDir}/exec_arm_little`,
         `${fixtureDir}/go_x86_64_both_gnu_and_go_build_id`,
         `${fixtureDir}/go_x86_64_only_go_build_id`,
-        `${fixtureDir}/go_x86_64_only_go_build_id.debug`,
       ])
     })
 
@@ -131,12 +130,17 @@ describe('elf-symbols upload', () => {
     test('should not throw an error when a directory (except top-level) is not readable', async () => {
       const command = createCommand(UploadCommand)
       let tmpDir
+      let tmpSubDir
       try {
         tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'elf-tests-'))
-        const tmpSubDir = fs.mkdtempSync(path.join(tmpDir, 'unreadable-'))
+        tmpSubDir = fs.mkdtempSync(path.join(tmpDir, 'unreadable-'))
         fs.chmodSync(tmpSubDir, 0o200)
         await expect(command['getElfSymbolFiles'](tmpDir)).resolves.toEqual([])
       } finally {
+        if (tmpSubDir) {
+          // node 23 fails to remove the directory if it's not readable
+          fs.chmodSync(tmpSubDir, 0o700)
+        }
         if (tmpDir) {
           fs.rmSync(tmpDir, {recursive: true})
         }
@@ -263,17 +267,17 @@ describe('elf-symbols upload', () => {
         },
         {
           ...commonMetadata,
-          file_hash: '',
+          file_hash: '5ba2907faebb8002de89711d5f0f005c',
           gnu_build_id: '90aef8b4a3cd45d758501e49d1d9844736c872cd',
           go_build_id: '',
           arch: 'aarch64',
-          filename: 'dyn_aarch64.debug',
+          filename: 'dyn_aarch64',
           symbol_source: 'debug_info',
         },
         {
           ...commonMetadata,
-          file_hash: 'e8a12b7f5702d7a4f92da4983d75e9af',
-          gnu_build_id: 'a8ac08faa0d114aa65f1ee0730af38903ac506de',
+          file_hash: '40a0f8378cf61c89c325a397edaa0dd2',
+          gnu_build_id: '90aef8b4a3cd45d758501e49d1d9844736c872cd',
           go_build_id: '',
           arch: 'x86_64',
           filename: 'dyn_x86_64',
@@ -317,7 +321,7 @@ describe('elf-symbols upload', () => {
 
         return JSON.parse((payload.content.get('event') as MultipartStringValue).value)
       })
-      const getId = (m: any) => m.gnu_build_id || m.go_build_id || m.file_hash
+      const getId = (m: any) => ((m.gnu_build_id || m.go_build_id || m.file_hash) as string) + '-' + (m.arch as string)
       metadata.sort((a, b) => getId(a).localeCompare(getId(b)))
       expectedMetadata.sort((a, b) => getId(a).localeCompare(getId(b)))
       expect(metadata).toEqual(expectedMetadata)

src/commands/elf-symbols/__tests__/upload.test.ts

@@ -421,12 +421,16 @@ export const isSupportedElfType = (type: string): boolean => {
   return SUPPORTED_ELF_TYPES.includes(type)
 }
 
+export const hasNonEmptySection = (sectionHeaders: SectionHeader[], name: string): boolean => {
+  return sectionHeaders.some((section) => section.name === name && section.sh_type !== SectionHeaderType.SHT_NOBITS)
+}
+
 export const getSectionInfo = (
   sections: SectionHeader[]
 ): {hasDebugInfo: boolean; hasSymbolTable: boolean; hasDynamicSymbolTable: boolean; hasCode: boolean} => {
-  const hasDebugInfo = sections.some((section) => section.name === '.debug_info')
-  const hasSymbolTable = sections.some((section) => section.name === '.symtab')
-  const hasDynamicSymbolTable = sections.some((section) => section.name === '.dynsym')
+  const hasDebugInfo = hasNonEmptySection(sections, '.debug_info') || hasNonEmptySection(sections, '.zdebug_info')
+  const hasSymbolTable = hasNonEmptySection(sections, '.symtab')
+  const hasDynamicSymbolTable = hasNonEmptySection(sections, '.dynsym')
   const hasCode = sections.some(
     (section) => section.name === '.text' && section.sh_type === SectionHeaderType.SHT_PROGBITS
   )
@@ -523,29 +527,32 @@ export const computeFileHash = async (filename: string): Promise<string> => {
   }
 }
 
-const getSupportedBfdTargetsInternal = async (): Promise<string[]> => {
+const hasZstdSupport = async (): Promise<boolean> => {
+  const {stdout} = await execute('objcopy --help')
+
+  return /--compress-debug-sections.*zstd/.test(stdout.toString())
+}
+
+const memoize = <T>(fn: () => Promise<T>): (() => Promise<T>) => {
+  let promise: Promise<T> | undefined
+
+  return () => (promise = promise || fn())
+}
+
+const hasZstdSupportCached = memoize(hasZstdSupport)
+
+const getSupportedBfdTargets = async (): Promise<string[]> => {
   const {stdout} = await execute('objcopy --help')
 
   const groups = /supported targets: (?<targets>.*)$/m.exec(stdout.toString())?.groups
   if (groups) {
-    return groups.targets.split(/\s*/)
+    return groups.targets.split(/\s+/)
   }
 
   return []
 }
 
-const getSupportedBfdTargets = (() => {
-  let promise: Promise<string[]> | undefined
-
-  return () =>
-    (promise =
-      promise ||
-      (async () => {
-        const targets = await getSupportedBfdTargetsInternal()
-
-        return targets
-      })())
-})()
+const getSupportedBfdTargetsCached = memoize(getSupportedBfdTargets)
 
 const replaceElfHeader = async (targetFilename: string, sourceFilename: string): Promise<void> => {
   const sourceElfHeader = await getElfHeaderStart(sourceFilename)
@@ -560,7 +567,7 @@ export const copyElfDebugInfo = async (
   elfFileMetadata: ElfFileMetadata,
   compressDebugSections: boolean
 ): Promise<void> => {
-  const supportedTargets = await getSupportedBfdTargets()
+  const supportedTargets = await getSupportedBfdTargetsCached()
 
   let bfdTargetOption = ''
   const bfdTarget = getBFDTargetForArch(elfFileMetadata.arch, elfFileMetadata.littleEndian, elfFileMetadata.elfClass)
@@ -574,15 +581,38 @@ export const copyElfDebugInfo = async (
     bfdTargetOption = `-I ${genericBfdTarget}`
   }
 
-  const compressDebugSectionsOption = compressDebugSections ? '--compress-debug-sections' : ''
+  let compressDebugSectionsOption = ''
+  if (compressDebugSections) {
+    compressDebugSectionsOption = '--compress-debug-sections'
+    if (await hasZstdSupportCached()) {
+      compressDebugSectionsOption += '=zstd'
+    }
+  }
+
+  const keepDynamicSymbolTable =
+    elfFileMetadata.hasDynamicSymbolTable && !elfFileMetadata.hasSymbolTable && !elfFileMetadata.hasDebugInfo
 
   // Remove .gdb_index section as it is not needed and can be quite big
-  await execute(
-    `objcopy ${bfdTargetOption} --only-keep-debug ${compressDebugSectionsOption} --remove-section=.gdb_index ${filename} ${outputFile}`
-  )
+  let options = `${bfdTargetOption} --only-keep-debug ${compressDebugSectionsOption} --remove-section=.gdb_index`
+
+  if (keepDynamicSymbolTable) {
+    // If the file has only a dynamic symbol table, preserve it
+    // `objcopy --only-keep-debug` would remove it, so we need to dump it separately with `objcopy --dump-section` and then merge it back with `objcopy --add-section`
+    await execute(
+      `objcopy ${bfdTargetOption} --dump-section .dynsym=${outputFile}.dynsym --dump-section .dynstr=${outputFile}.dynstr ${filename} ${outputFile}`
+    )
+    options = `--remove-section .dynsym --remove-section .dynstr ${options} --add-section .dynsym=${outputFile}.dynsym --add-section .dynstr=${outputFile}.dynstr`
+  }
+
+  await execute(`objcopy ${options} ${filename} ${outputFile}`).finally(() => {
+    if (keepDynamicSymbolTable) {
+      fs.unlinkSync(`${outputFile}.dynsym`)
+      fs.unlinkSync(`${outputFile}.dynstr`)
+    }
+  })
 
   if (bfdTargetOption) {
-    // Replace the ELF header in the extracted debug info file with the one from the initial file
+    // Replace the ELF header in the extracted debug info file with the one from the initial file to keep the original architecture
     await replaceElfHeader(outputFile, filename)
   }
 }
@@ -592,6 +622,6 @@ export const getOutputFilenameFromBuildId = (buildId: string): string => {
   return buildId.replace(/\//g, '-')
 }
 
-export const getBuildId = (fileMetadata: ElfFileMetadata): string => {
-  return fileMetadata.gnuBuildId || fileMetadata.goBuildId || fileMetadata.fileHash
+export const getBuildIdWithArch = (fileMetadata: ElfFileMetadata): string => {
+  return (fileMetadata.gnuBuildId || fileMetadata.goBuildId || fileMetadata.fileHash) + '-' + fileMetadata.arch
 }

src/commands/elf-symbols/elf.ts

@@ -23,7 +23,7 @@ import {
   ElfFileMetadata,
   getElfFileMetadata,
   isSupportedElfType,
-  getBuildId,
+  getBuildIdWithArch,
   getOutputFilenameFromBuildId,
   copyElfDebugInfo,
   isSupportedArch,
@@ -282,7 +282,7 @@ export class UploadCommand extends Command {
   private removeBuildIdDuplicates(filesMetadata: ElfFileMetadata[]): ElfFileMetadata[] {
     const buildIds = new Map<string, ElfFileMetadata>()
     for (const metadata of filesMetadata) {
-      const buildId = getBuildId(metadata)
+      const buildId = getBuildIdWithArch(metadata)
       const existing = buildIds.get(buildId)
       if (existing) {
         if (
@@ -328,9 +328,9 @@ export class UploadCommand extends Command {
     try {
       const results = await doWithMaxConcurrency(this.maxConcurrency, elfFilesMetadata, async (fileMetadata) => {
         const metadata = this.getMappingMetadata(fileMetadata)
-        const outputFilename = getOutputFilenameFromBuildId(getBuildId(fileMetadata))
+        const outputFilename = getOutputFilenameFromBuildId(getBuildIdWithArch(fileMetadata))
         const outputFilePath = buildPath(tmpDirectory, outputFilename)
-        await copyElfDebugInfo(fileMetadata.filename, outputFilePath, fileMetadata, false)
+        await copyElfDebugInfo(fileMetadata.filename, outputFilePath, fileMetadata, true)
 
         if (this.dryRun) {
           this.context.stdout.write(`[DRYRUN] ${renderUpload(fileMetadata.filename, metadata)}`)