[llvm] r364254 - [llvm-objcopy][NFC] Refactor output target parsing
Jordan Rupprecht via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 28 11:39:58 PDT 2019
Seiya, were you able to reproduce the issue locally to recommit the patch?
On Tue, Jun 25, 2019 at 7:40 PM Rumeet Dhindsa via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> 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
>>
> _______________________________________________
> 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/20190628/aec4e8ec/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4849 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190628/aec4e8ec/attachment-0001.bin>
More information about the llvm-commits
mailing list