[PATCH] D42872: Fix handling of zero-size segments in llvm-objcopy

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-objcopy/Object.cpp:916-923
+    // to signalize about feature support or similar (e.g. GNU_STACK).
+    // These segments do not have any sections, and have 0 FileSize.
+    // Other than that their Offset, VAddr, PAddr, and MemSize are also 0.
+    // Including such a segment in OrderedSegments will produce the lowest
+    // offset of 0, and will let the following segments overwrite ELF header.
+    // Here we just check against FileSize as most sensible, since non-zero
+    // MemSize is technically valid, and non-zero Offset should probably
----------------
signalize about feature support -> signal support of a feature
overwrite ELF header -> overwrite the ELF header

The last sentence will also need updating once the check is changed (see below comment). I would assume that non-zero memsize of marker segments is not valid, since they won't contribute to the program image.


================
Comment at: tools/llvm-objcopy/Object.cpp:924
+    // not affect any other segments if FileSize is 0 too.
+    if (Segment.FileSize > 0)
+      OrderedSegments.push_back(&Segment);
----------------
This still doesn't work for a PT_LOAD data segment containing .bss only. With the code as-is, such a segment will not be added to OrderedSegments, and so will never have its offset adjusted, meaning that the offset will be incorrect. Such segments should have the conceptual offset of where they lie in the file. This affects things like dumping tools that try to map sections to segments, based on file layout.

Please add to your test such a segment (i.e. one with zero FileSize, non-zero MemSize).


Repository:
  rL LLVM

https://reviews.llvm.org/D42872