[llvm] r364254 - [llvm-objcopy][NFC] Refactor output target parsing
Rumeet Dhindsa via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 25 19:39:46 PDT 2019
With this patch, some of llvm-objcopy tests are failing with valgrind.
Following is the output for basic-keep.test
Command Output (stderr):
--
==107406== Conditional jump or move depends on uninitialised value(s)
==107406== at 0x1A30DD: executeObjcopy(llvm::objcopy::CopyConfig const&)
(llvm-objcopy.cpp:235)
==107406== by 0x1A3935: main (llvm-objcopy.cpp:294)
==107406==
==107406== Conditional jump or move depends on uninitialised value(s)
==107406== at 0x1A30E2: executeObjcopy(llvm::objcopy::CopyConfig const&)
(llvm-objcopy.cpp:235)
==107406== by 0x1A3935: main (llvm-objcopy.cpp:294)
==107406==
==107406== Conditional jump or move depends on uninitialised value(s)
==107406== at 0x1C1518:
llvm::objcopy::elf::createWriter(llvm::objcopy::CopyConfig const&,
llvm::objcopy::elf::Object&, llvm::objcopy::Buffer&,
llvm::objcopy::elf::ElfType) (ELFObjcopy.cpp:157)
==107406== by 0x1C50DE:
llvm::objcopy::elf::writeOutput(llvm::objcopy::CopyConfig const&,
llvm::objcopy::elf::Object&, llvm::objcopy::Buffer&,
llvm::objcopy::elf::ElfType) (ELFObjcopy.cpp:740)
==107406== by 0x1C587F:
llvm::objcopy::elf::executeObjcopyOnBinary(llvm::objcopy::CopyConfig
const&, llvm::object::ELFObjectFileBase&, llvm::objcopy::Buffer&)
(ELFObjcopy.cpp:803)
==107406== by 0x1A25F2: executeObjcopyOnBinary(llvm::objcopy::CopyConfig
const&, llvm::object::Binary&, llvm::objcopy::Buffer&)
(llvm-objcopy.cpp:162)
==107406== by 0x1A3445: executeObjcopy(llvm::objcopy::CopyConfig const&)
(llvm-objcopy.cpp:265)
==107406== by 0x1A3935: main (llvm-objcopy.cpp:294)
==107406==
==107406== Conditional jump or move depends on uninitialised value(s)
==107406== at 0x1C151D:
llvm::objcopy::elf::createWriter(llvm::objcopy::CopyConfig const&,
llvm::objcopy::elf::Object&, llvm::objcopy::Buffer&,
llvm::objcopy::elf::ElfType) (ELFObjcopy.cpp:157)
==107406== by 0x1C50DE:
llvm::objcopy::elf::writeOutput(llvm::objcopy::CopyConfig const&,
llvm::objcopy::elf::Object&, llvm::objcopy::Buffer&,
llvm::objcopy::elf::ElfType) (ELFObjcopy.cpp:740)
==107406== by 0x1C587F:
llvm::objcopy::elf::executeObjcopyOnBinary(llvm::objcopy::CopyConfig
const&, llvm::object::ELFObjectFileBase&, llvm::objcopy::Buffer&)
(ELFObjcopy.cpp:803)
==107406== by 0x1A25F2: executeObjcopyOnBinary(llvm::objcopy::CopyConfig
const&, llvm::object::Binary&, llvm::objcopy::Buffer&)
(llvm-objcopy.cpp:162)
==107406== by 0x1A3445: executeObjcopy(llvm::objcopy::CopyConfig const&)
(llvm-objcopy.cpp:265)
==107406== by 0x1A3935: main (llvm-objcopy.cpp:294)
==107406==
--
Failing Tests:
llvm-objcopy/ELF/basic-keep.test
llvm-objcopy/ELF/basic-only-keep-debug.test
llvm-objcopy/ELF/discard-all-debug.test
llvm-objcopy/ELF/discard-all.test
llvm-objcopy/ELF/discard-locals.test
llvm-objcopy/ELF/discard-mix-local-and-all.test
llvm-objcopy/ELF/dynsym-error-remove-strtab.test
llvm-objcopy/ELF/keep-file-symbols.test
llvm-objcopy/ELF/no-strip-all.test
llvm-objcopy/ELF/reloc-error-remove-symtab.test
llvm-objcopy/ELF/reloc-no-symtab.test
llvm-objcopy/ELF/remove-linked-section.test
llvm-objcopy/ELF/same-file-strip.test
llvm-objcopy/ELF/strip-all-and-keep-symbol.test
llvm-objcopy/ELF/strip-all-and-remove.test
llvm-objcopy/ELF/strip-all-gnu.test
llvm-objcopy/ELF/strip-all.test
llvm-objcopy/ELF/strip-debug-and-remove.test
llvm-objcopy/ELF/strip-debug.test
llvm-objcopy/ELF/strip-multiple-files.test
llvm-objcopy/ELF/strip-preserve-atime.test
llvm-objcopy/ELF/strip-preserve-mtime.test
llvm-objcopy/ELF/strip-symbol.test
llvm-objcopy/ELF/strip-unneeded.test
llvm-objcopy/ELF/symtab-error-on-remove-strtab.test
llvm-objcopy/ELF/symtab-link.test
I verified locally that by reverting this change, the tests are passing.
On Mon, Jun 24, 2019 at 5:02 PM Seiya Nuta via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> Author: seiya
> Date: Mon Jun 24 17:02:04 2019
> New Revision: 364254
>
> URL: http://llvm.org/viewvc/llvm-project?rev=364254&view=rev
> Log:
> [llvm-objcopy][NFC] Refactor output target parsing
>
> Summary:
> Use an enum instead of string to hold the output file format in
> Config.InputFormat and Config.OutputFormat. It's essential to support other
> output file formats other than ELF.
>
> Reviewers: espindola, alexshap, rupprecht, jhenderson
>
> Reviewed By: rupprecht, jhenderson
>
> Subscribers: jyknight, compnerd, emaste, arichardson, fedor.sergeev,
> jakehehrlich, MaskRay, llvm-commits
>
> Tags: #llvm
>
> Differential Revision: https://reviews.llvm.org/D63239
>
> Modified:
> llvm/trunk/tools/llvm-objcopy/CopyConfig.cpp
> llvm/trunk/tools/llvm-objcopy/CopyConfig.h
> llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
> llvm/trunk/tools/llvm-objcopy/llvm-objcopy.cpp
>
> Modified: llvm/trunk/tools/llvm-objcopy/CopyConfig.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objcopy/CopyConfig.cpp?rev=364254&r1=364253&r2=364254&view=diff
>
> ==============================================================================
> --- llvm/trunk/tools/llvm-objcopy/CopyConfig.cpp (original)
> +++ llvm/trunk/tools/llvm-objcopy/CopyConfig.cpp Mon Jun 24 17:02:04 2019
> @@ -277,8 +277,13 @@ static Expected<const MachineInfo &> get
> return Iter->getValue();
> }
>
> +struct TargetInfo {
> + FileFormat Format;
> + MachineInfo Machine;
> +};
> +
> // FIXME: consolidate with the bfd parsing used by lld.
> -static const StringMap<MachineInfo> OutputFormatMap{
> +static const StringMap<MachineInfo> TargetMap{
> // Name, {EMachine, 64bit, LittleEndian}
> // x86
> {"elf32-i386", {ELF::EM_386, false, true}},
> @@ -312,18 +317,28 @@ static const StringMap<MachineInfo> Outp
> {"elf32-sparcel", {ELF::EM_SPARC, false, true}},
> };
>
> -static Expected<MachineInfo> getOutputFormatMachineInfo(StringRef Format)
> {
> - StringRef OriginalFormat = Format;
> - bool IsFreeBSD = Format.consume_back("-freebsd");
> - auto Iter = OutputFormatMap.find(Format);
> - if (Iter == std::end(OutputFormatMap))
> +static Expected<TargetInfo>
> +getOutputTargetInfoByTargetName(StringRef TargetName) {
> + StringRef OriginalTargetName = TargetName;
> + bool IsFreeBSD = TargetName.consume_back("-freebsd");
> + auto Iter = TargetMap.find(TargetName);
> + if (Iter == std::end(TargetMap))
> return createStringError(errc::invalid_argument,
> "invalid output format: '%s'",
> - OriginalFormat.str().c_str());
> + OriginalTargetName.str().c_str());
> MachineInfo MI = Iter->getValue();
> if (IsFreeBSD)
> MI.OSABI = ELF::ELFOSABI_FREEBSD;
> - return {MI};
> +
> + FileFormat Format;
> + if (TargetName.startswith("elf"))
> + Format = FileFormat::ELF;
> + else
> + // This should never happen because `TargetName` is valid (it
> certainly
> + // exists in the TargetMap).
> + llvm_unreachable("unknown target prefix");
> +
> + return {TargetInfo{Format, MI}};
> }
>
> static Error addSymbolsFromFile(std::vector<NameOrRegex> &Symbols,
> @@ -445,14 +460,23 @@ Expected<DriverConfig> parseObjcopyOptio
> "--target cannot be used with --input-target or --output-target");
>
> bool UseRegex = InputArgs.hasArg(OBJCOPY_regex);
> + StringRef InputFormat, OutputFormat;
> if (InputArgs.hasArg(OBJCOPY_target)) {
> - Config.InputFormat = InputArgs.getLastArgValue(OBJCOPY_target);
> - Config.OutputFormat = InputArgs.getLastArgValue(OBJCOPY_target);
> + InputFormat = InputArgs.getLastArgValue(OBJCOPY_target);
> + OutputFormat = InputArgs.getLastArgValue(OBJCOPY_target);
> } else {
> - Config.InputFormat = InputArgs.getLastArgValue(OBJCOPY_input_target);
> - Config.OutputFormat =
> InputArgs.getLastArgValue(OBJCOPY_output_target);
> + InputFormat = InputArgs.getLastArgValue(OBJCOPY_input_target);
> + OutputFormat = InputArgs.getLastArgValue(OBJCOPY_output_target);
> }
> - if (Config.InputFormat == "binary") {
> +
> + // FIXME: Currently, we ignore the target for non-binary/ihex formats
> + // explicitly specified by -I option (e.g. -Ielf32-x86-64) and guess the
> + // format by llvm::object::createBinary regardless of the option value.
> + Config.InputFormat = StringSwitch<FileFormat>(InputFormat)
> + .Case("binary", FileFormat::Binary)
> + .Case("ihex", FileFormat::IHex)
> + .Default(FileFormat::Unspecified);
> + if (Config.InputFormat == FileFormat::Binary) {
> auto BinaryArch =
> InputArgs.getLastArgValue(OBJCOPY_binary_architecture);
> if (BinaryArch.empty())
> return createStringError(
> @@ -463,12 +487,17 @@ Expected<DriverConfig> parseObjcopyOptio
> return MI.takeError();
> Config.BinaryArch = *MI;
> }
> - if (!Config.OutputFormat.empty() && Config.OutputFormat != "binary" &&
> - Config.OutputFormat != "ihex") {
> - Expected<MachineInfo> MI =
> getOutputFormatMachineInfo(Config.OutputFormat);
> - if (!MI)
> - return MI.takeError();
> - Config.OutputArch = *MI;
> +
> + Config.OutputFormat = StringSwitch<FileFormat>(OutputFormat)
> + .Case("binary", FileFormat::Binary)
> + .Case("ihex", FileFormat::IHex)
> + .Default(FileFormat::Unspecified);
> + if (Config.OutputFormat == FileFormat::Unspecified &&
> !OutputFormat.empty()) {
> + Expected<TargetInfo> Target =
> getOutputTargetInfoByTargetName(OutputFormat);
> + if (!Target)
> + return Target.takeError();
> + Config.OutputFormat = Target->Format;
> + Config.OutputArch = Target->Machine;
> }
>
> if (auto Arg = InputArgs.getLastArg(OBJCOPY_compress_debug_sections,
>
> Modified: llvm/trunk/tools/llvm-objcopy/CopyConfig.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objcopy/CopyConfig.h?rev=364254&r1=364253&r2=364254&view=diff
>
> ==============================================================================
> --- llvm/trunk/tools/llvm-objcopy/CopyConfig.h (original)
> +++ llvm/trunk/tools/llvm-objcopy/CopyConfig.h Mon Jun 24 17:02:04 2019
> @@ -26,6 +26,13 @@
> namespace llvm {
> namespace objcopy {
>
> +enum class FileFormat {
> + Unspecified,
> + ELF,
> + Binary,
> + IHex,
> +};
> +
> // This type keeps track of the machine info for various architectures.
> This
> // lets us map architecture names to ELF types and the e_machine value of
> the
> // ELF file.
> @@ -104,9 +111,9 @@ struct NewSymbolInfo {
> struct CopyConfig {
> // Main input/output options
> StringRef InputFilename;
> - StringRef InputFormat;
> + FileFormat InputFormat;
> StringRef OutputFilename;
> - StringRef OutputFormat;
> + FileFormat OutputFormat;
>
> // Only applicable for --input-format=binary
> MachineInfo BinaryArch;
>
> Modified: llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp?rev=364254&r1=364253&r2=364254&view=diff
>
> ==============================================================================
> --- llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp (original)
> +++ llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp Mon Jun 24 17:02:04
> 2019
> @@ -154,12 +154,14 @@ static std::unique_ptr<Writer> createELF
> static std::unique_ptr<Writer> createWriter(const CopyConfig &Config,
> Object &Obj, Buffer &Buf,
> ElfType OutputElfType) {
> - using Functor = std::function<std::unique_ptr<Writer>()>;
> - return StringSwitch<Functor>(Config.OutputFormat)
> - .Case("binary", [&] { return llvm::make_unique<BinaryWriter>(Obj,
> Buf); })
> - .Case("ihex", [&] { return llvm::make_unique<IHexWriter>(Obj, Buf);
> })
> - .Default(
> - [&] { return createELFWriter(Config, Obj, Buf, OutputElfType);
> })();
> + switch (Config.OutputFormat) {
> + case FileFormat::Binary:
> + return llvm::make_unique<BinaryWriter>(Obj, Buf);
> + case FileFormat::IHex:
> + return llvm::make_unique<IHexWriter>(Obj, Buf);
> + default:
> + return createELFWriter(Config, Obj, Buf, OutputElfType);
> + }
> }
>
> template <class ELFT>
>
> Modified: llvm/trunk/tools/llvm-objcopy/llvm-objcopy.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objcopy/llvm-objcopy.cpp?rev=364254&r1=364253&r2=364254&view=diff
>
> ==============================================================================
> --- llvm/trunk/tools/llvm-objcopy/llvm-objcopy.cpp (original)
> +++ llvm/trunk/tools/llvm-objcopy/llvm-objcopy.cpp Mon Jun 24 17:02:04 2019
> @@ -140,11 +140,16 @@ static Error executeObjcopyOnIHex(const
> /// of the output specified by the command line options.
> static Error executeObjcopyOnRawBinary(const CopyConfig &Config,
> MemoryBuffer &In, Buffer &Out) {
> - // TODO: llvm-objcopy should parse CopyConfig.OutputFormat to recognize
> - // formats other than ELF / "binary" and invoke
> - // elf::executeObjcopyOnRawBinary, macho::executeObjcopyOnRawBinary or
> - // coff::executeObjcopyOnRawBinary accordingly.
> - return elf::executeObjcopyOnRawBinary(Config, In, Out);
> + switch (Config.OutputFormat) {
> + case FileFormat::ELF:
> + // FIXME: Currently, we call elf::executeObjcopyOnRawBinary even if the
> + // output format is binary/ihex or it's not given. This behavior
> differs from
> + // GNU objcopy. See https://bugs.llvm.org/show_bug.cgi?id=42171 for
> details.
> + case FileFormat::Binary:
> + case FileFormat::IHex:
> + case FileFormat::Unspecified:
> + return elf::executeObjcopyOnRawBinary(Config, In, Out);
> + }
> }
>
> /// The function executeObjcopyOnBinary does the dispatch based on the
> format
> @@ -224,10 +229,17 @@ static Error executeObjcopy(const CopyCo
> return createFileError(Config.InputFilename, EC);
>
> typedef Error (*ProcessRawFn)(const CopyConfig &, MemoryBuffer &,
> Buffer &);
> - auto ProcessRaw = StringSwitch<ProcessRawFn>(Config.InputFormat)
> - .Case("binary", executeObjcopyOnRawBinary)
> - .Case("ihex", executeObjcopyOnIHex)
> - .Default(nullptr);
> + ProcessRawFn ProcessRaw;
> + switch (Config.InputFormat) {
> + case FileFormat::Binary:
> + ProcessRaw = executeObjcopyOnRawBinary;
> + break;
> + case FileFormat::IHex:
> + ProcessRaw = executeObjcopyOnIHex;
> + break;
> + default:
> + ProcessRaw = nullptr;
> + }
>
> if (ProcessRaw) {
> auto BufOrErr = MemoryBuffer::getFileOrSTDIN(Config.InputFilename);
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190625/84353c98/attachment-0001.html>
More information about the llvm-commits
mailing list