[llvm] r364254 - [llvm-objcopy][NFC] Refactor output target parsing
Rumeet Dhindsa via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 28 13:55:19 PDT 2019
Hi Seiya,
To test with valgrind, you will need to add --vg to the lit test command.
llvm-lit --vg llvm-project/llvm/test/tools/llvm-objcopy/ELF/basic-keep.test
Thanks,
Rumeet
On Fri, Jun 28, 2019 at 11:40 AM Jordan Rupprecht <rupprecht at google.com>
wrote:
> 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/afd608cd/attachment.html>
More information about the llvm-commits
mailing list