[llvm] r364254 - [llvm-objcopy][NFC] Refactor output target parsing
Seiya Nuta via llvm-commits
llvm-commits at lists.llvm.org
Sat Jun 29 05:12:05 PDT 2019
Hi Jordan and Rumeet,
Thank you for letting me know about the bug! I'll try that cool --vg option.
I haven't tried but I suspect that it is caused by not setting
Config.InputFormat in parseStripOptions. It seems that basic-keep.test
doesn't invoke llvm-objcopy as strip so it could be a separate issue
though.
Unfortunately, I cannot check it out now (I don't have a computer to
build LLVM) so I'll submit an updated patch on Monday.
Thank you,
Seiya
On Sat, Jun 29, 2019 at 5:55 AM Rumeet Dhindsa <rdhindsa at google.com> wrote:
>
> 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
More information about the llvm-commits
mailing list