This is an archive of the discontinued LLVM Phabricator instance.

[AsmParser] Add support to LOCAL directive.
AbandonedPublic

Authored by jcai19 on Jul 9 2021, 12:13 PM.

Details

Summary

This addresses PR#45051 and addes support to LOCAL directive when
alternate macro mode is enabled.

Link:
https://sourceware.org/binutils/docs/as/Altmacro.html#Altmacro

Diff Detail

Event Timeline

jcai19 created this revision.Jul 9 2021, 12:13 PM
jcai19 requested review of this revision.Jul 9 2021, 12:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2021, 12:13 PM
jcai19 added subscribers: manojgupta, llozano.
nickdesaulniers added inline comments.Jul 9 2021, 1:36 PM
llvm/lib/MC/MCParser/AsmParser.cpp
2504

would it be preferable to use llvm::StringSet?

https://llvm.org/docs/ProgrammersManual.html#llvm-adt-stringset-h

jcai19 updated this revision to Diff 357647.Jul 9 2021, 2:51 PM

Use StringSet instead of std::unordered_set <std::string>.

llvm/lib/MC/MCParser/AsmParser.cpp
2504

Sounds good! Thanks.

MaskRay requested changes to this revision.Jul 12 2021, 2:41 PM

DenseSet<CachedHashStringRef> may be better than StringSet. The former doesn't need to duplicate the string.

Does LOCAL (https://bugs.llvm.org//show_bug.cgi?id=45051) have a use case?
As you mentioned, this can usually be replaced with local labels (1:)


If the feature turns out to be useful,

The implementation looks hackish to me. A proper implemention should make the LOCAL symbol similar to a macro argument.

I think the replacement should use a .LL symbol instead. For non-RISC-V the local symbols are not intended to be retained in the symbol table.

This revision now requires changes to proceed.Jul 12 2021, 2:41 PM
jcai19 added a comment.EditedJul 13 2021, 4:12 PM

DenseSet<CachedHashStringRef> may be better than StringSet. The former doesn't need to duplicate the string.

Thanks!

Does LOCAL (https://bugs.llvm.org//show_bug.cgi?id=45051) have a use case?
As you mentioned, this can usually be replaced with local labels (1:)

Yes, the reported case should be fixed by the proposed workaround but sometimes it may be more difficult to find a workaround such as in the the test cases. While this might not be common, PR#45051 shows there still could be real-world use cases of this directive, and I think we should try to support it especially when it does not introduce significant change to the existing code.


If the feature turns out to be useful,

The implementation looks hackish to me. A proper implemention should make the LOCAL symbol similar to a macro argument.

That would look cleaner but if I understand your comment correctly, it is not easy to integrate the code into the following loop that handles macro arguments. For macro arguments, IAS could easily identify their uses by checking \. This does not apply to uses of LOCAL arguments, and if we would integrate the code, we need to check against possible LOCAL arguments at every character of Body, which would be more expensive. The currently implementation goes through each use case of LOCAL arguments once (after it identifies the corresponding LOCAL directive call), while integrating it into the for loop would introduce (length of Body * possible LOCAL arguments) checks. WDYT?

I think the replacement should use a .LL symbol instead. For non-RISC-V the local symbols are not intended to be retained in the symbol table.

Agreed, will update my code.

Yes, the reported case should be fixed by the proposed workaround but sometimes it may be more difficult to find a workaround such as in the the test cases. While this might not be common, PR#45051 shows there still could be real-work use cases of this directive, and I think we should try to support it especially when it does not introduce significant change to the existing code.

The PR doesn't list a use case. This could be an obscure feature which probably no code uses.
(There are now ~80 lines so I am reluctant on accepting that this is insignificant amount of code.)

jcai19 added a comment.EditedJul 13 2021, 5:22 PM

Yes, the reported case should be fixed by the proposed workaround but sometimes it may be more difficult to find a workaround such as in the the test cases. While this might not be common, PR#45051 shows there still could be real-work use cases of this directive, and I think we should try to support it especially when it does not introduce significant change to the existing code.

The PR doesn't list a use case. This could be an obscure feature which probably no code uses.
(There are now ~80 lines so I am reluctant on accepting that this is insignificant amount of code.)

RP#4505 stated this issue happened while building risc-v kernel with IAS so I assume it was real-world usage. @nickdesaulniers is there an associated CBL link with more information? Thanks.

jcai19 updated this revision to Diff 359470.EditedJul 16 2021, 4:02 PM

On second thought, my earlier analysis regarding the time complexity was not correct. Integrating the implementation of LOCAL directive into the for loop that handles macro arguments does not make it more expensive. This implements LOCAL directive within the for loop and avoids copying the macro body, and hopefully is more consistent with the existing code. This also reduces the number of lines added.

Friendly ping.

I still don't see a sufficient justification for making the change.

I still don't see a sufficient justification for making the change.

Would supporting this directive to match GNU spec be a sufficient justification? I've reduced the implementation to 50 lines of code, so hopefully this is a less significant change compared to my initial implementation. I also found many tests in GAS test suite for this directive, so this should also help if anyone is interested in running GAS test suite with IAS to assess the incompatibility between IAS and GAS.

jcai19 abandoned this revision.Sep 13 2021, 2:16 PM

Actually the coreboot project (open source firmware) uses the 'LOCAL' directive: https://review.coreboot.org/c/coreboot/+/63045/ .
I'm trying to get that project working and building with clang so this feature would certainly help.

Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2022, 12:55 PM
Herald added a subscriber: StephenFan. · View Herald Transcript