This is an archive of the discontinued LLVM Phabricator instance.

[ObjectYAML] Add support for error handling in DWARFYAML. NFC.
ClosedPublic

Authored by Higuoxing on Jun 7 2020, 8:51 PM.

Details

Summary

This patch intends to be an NFC-patch. Test cases will be added in a follow-up patch.

Diff Detail

Event Timeline

Higuoxing created this revision.Jun 7 2020, 8:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2020, 8:51 PM

To silence all the clang-tidy warnings, I'd be happy if you want to do a bulk renaming of the functions (to emitDebug*) as a separate commit before this patch. Since you're going to modify all those lines in this patch anyway, it'll not make git blame any worse, which is usually the motivation behind not renaming things.

That being said, do you actually need to make all the functions return Error? Do you anticipate them all getting reporting some form of error? If you don't, making them return Error complicates the usage and adds some code paths (i.e. a function returning a failed Error) we can't test, because they are dead. Perhaps it would be better to only modify the signature of EmitDebugRanges, since that's where you need the error reported from.

llvm/lib/ObjectYAML/ELFEmitter.cpp
894–896

Rather than just writing handleAllErrors here, I'd move this into a helper function alongside reportError, probably as a reportError overload that takes Error. That would be useful going forwards.

Higuoxing marked an inline comment as done.Jun 8 2020, 1:30 AM

To silence all the clang-tidy warnings, I'd be happy if you want to do a bulk renaming of the functions (to emitDebug*) as a separate commit before this patch. Since you're going to modify all those lines in this patch anyway, it'll not make git blame any worse, which is usually the motivation behind not renaming things.

Sure, I will do it later.

That being said, do you actually need to make all the functions return Error? Do you anticipate them all getting reporting some form of error? If you don't, making them return Error complicates the usage and adds some code paths (i.e. a function returning a failed Error) we can't test, because they are dead. Perhaps it would be better to only modify the signature of EmitDebugRanges, since that's where you need the error reported from.

Yes, I need to, but I'm not sure if I really need to. There's a helper function called EmitDebugSectionImpl that takes EmitFuncType as the parameter, where EmitFuncType can be EmitDebugStrings, EmitDebugARanges etc. The EmitDebugXXX functions have to have same return type (See my inline comment).

llvm/lib/ObjectYAML/DWARFEmitter.cpp
333

The EmitDebugXXXX functions have to have same type.

Okay, approach looks fine. Just my comments about function renaming and the error handler to address then.

llvm/lib/ObjectYAML/DWARFEmitter.cpp
333

Right, I see, thanks!

grimar added inline comments.Jun 8 2020, 3:34 AM
llvm/lib/ObjectYAML/ELFEmitter.cpp
863

My concern this starts to look a bit bulky. Perhaps something like the following would be better?

uint64_t BeginOffset = OS.tell();
Error Err = Error::success();

if (Name == ".debug_str")
  Err = DWARFYAML::EmitDebugStr(OS, DWARF);
else if (Name == ".debug_aranges")
  Err = DWARFYAML::EmitDebugAranges(OS, DWARF);
...

if (Err)
  return std::move(Err);
Higuoxing updated this revision to Diff 269166.Jun 8 2020, 4:23 AM
Higuoxing marked 3 inline comments as done.

Address comments.

llvm/lib/ObjectYAML/ELFEmitter.cpp
863

Thanks a lot!

Higuoxing updated this revision to Diff 269179.Jun 8 2020, 4:49 AM

Apply joinErrors() to other code paths.

jhenderson added inline comments.Jun 8 2020, 5:56 AM
llvm/lib/ObjectYAML/DWARFEmitter.cpp
411–431

I think you could simplify this slightly:

Error Err = emitDebugSectionImpl(DI, &DWARFYAML::emitDebugInfo,
                                        "debug_info", DebugSections));
llvm/lib/ObjectYAML/ELFEmitter.cpp
863

Using joinErrors here and below doesn't make much sense, since there is only a single error to worry about. However, I believe you can't just assign to Err here either, as the original Err will be unchecked (even Error::success() needs to be checked before being overwritten/destroyed).

I think the thing to do is close to what @grimar suggested, but with one additional line:

Error Err = Error::success();
cantFail(Err); // Mark as checked before assigning.

if (Name == ".debug_str")
  Err = DWARFYAML::EmitDebugStr(OS, DWARF);
else if (Name == ".debug_aranges")
  Err = DWARFYAML::EmitDebugAranges(OS, DWARF);
...

if (Err)
  return std::move(Err);
llvm/lib/ObjectYAML/MachOEmitter.cpp
289–300

Same comment as above - all the joinErrors calls don't need to happen, since there is only ever one Error to worry about.

Higuoxing updated this revision to Diff 269194.Jun 8 2020, 6:12 AM
Higuoxing marked 4 inline comments as done.

Address comments.

Thanks for reviewing!

llvm/lib/ObjectYAML/ELFEmitter.cpp
863

Oh, thanks a lot. At first, I don't know how to make the Error::success() get checked. So I use joinErrors() everywhere.

grimar added inline comments.Jun 8 2020, 6:16 AM
llvm/lib/ObjectYAML/ELFEmitter.cpp
863

However, I believe you can't just assign to Err here either, as the original Err will be unchecked (even Error::success() needs to be checked before being overwritten/destroyed).

I think the following code will invoke and trigger the original Err to be checked:

/// Move-construct an error value. The newly constructed error is considered
/// unchecked, even if the source error had been checked. The original error
/// becomes a checked Success value, regardless of its original state.
Error(Error &&Other) {
  setChecked(true);
  *this = std::move(Other);
}
grimar added inline comments.Jun 8 2020, 6:17 AM
llvm/lib/ObjectYAML/ELFEmitter.cpp
863

I think the following code will invoke and trigger the original Err to be checked:

Am I wrong?

grimar added inline comments.Jun 8 2020, 6:19 AM
llvm/lib/ObjectYAML/ELFEmitter.cpp
863

Ah, I see I am wrong now. Nevermind.

jhenderson accepted this revision.Jun 8 2020, 6:35 AM

LGTM, but please wait for @grimar to confirm too.

This revision is now accepted and ready to land.Jun 8 2020, 6:35 AM
grimar accepted this revision.Jun 8 2020, 7:14 AM

LGTM

This revision was automatically updated to reflect the committed changes.