Page MenuHomePhabricator

[NFC][object] Change the input parameter of the method isDebugSection.
ClosedPublic

Authored by Esme on May 17 2021, 2:15 AM.

Details

Summary

This is a NFC patch to change the input parameter of the method SectionRef::isDebugSection(), by replacing the StringRef SectionName with DataRefImpl Sec. This allows us to determine if a section is debug type in more ways than just by section name.

Diff Detail

Event Timeline

Esme created this revision.May 17 2021, 2:15 AM
Esme requested review of this revision.May 17 2021, 2:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2021, 2:15 AM
qiucf added a subscriber: qiucf.May 17 2021, 2:41 AM

You added implementation of XCOFFObjectFile::isDebugSection, which does not exist before your patch (besides, its implementation is not as 'trivial' as others). Are you sure this is an NFC?

Esme added a comment.May 17 2021, 8:01 AM

You added implementation of XCOFFObjectFile::isDebugSection, which does not exist before your patch (besides, its implementation is not as 'trivial' as others). Are you sure this is an NFC?

I understand your concern, hmm..., I will move this part to the child revision D102603 where this method is really used. Thanks!

jhenderson added inline comments.May 18 2021, 1:10 AM
llvm/lib/Object/MachOObjectFile.cpp
2050

I recognise that this is the common pattern used elsewhere in this file, but if I'm not mistaken, it can lead to an unhandled Error should getSectionName ever fail, which is an assertion when certain options are enabled.

At the very least, there needs to be a consumeError(SectionNameOrErr.takeError()) line after the if, with a TODO saying to report the error message properly. If you can prove that getSectionName(Sec) never returns an error, you can use cantFail and avoid the TODO instead.

Same applies to each of the other formats.

llvm/lib/Object/ObjectFile.cpp
96–97

Nit: clang-format as directed.

Esme updated this revision to Diff 346148.May 18 2021, 5:31 AM

Address comments.

jhenderson added inline comments.May 19 2021, 12:46 AM
llvm/lib/Object/COFFObjectFile.cpp
335

You need to return after this consumeError, as you can't safely retrieve the name from a failed Expected. Same applies in the other file formats.

Esme updated this revision to Diff 347319.May 24 2021, 12:20 AM

Address comment.

This revision is now accepted and ready to land.May 24 2021, 1:30 AM