[cfe-dev] address_space pragma

Tue Aug 7 16:28:43 PDT 2018

On Tue, 7 Aug 2018 at 15:07, Leonard Chan via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

> I currently have an implementation that uses
> Preprocessor::getLastMacroWithSpelling to skim through the macro
> definition tokens for finding __attribute__ and address_space, but
> based off the unittests for Lexer::getImmediateMacroName, that looks
> like it could also work.
>
> The main issue I'm running into is that both of these require a
> SourceLocation, which doesn't seem to be available when printing the
> Qualifier in the typeprinter. So the only way I can actually use the
> macro in the typeprinter is by saving either the SourceLocation or
> macro name in the qualifier, which in turn is created through calls to
> ASTContext::getAddrSpaceQualType.
>
> My initial guess for tackling this is changing getAddrSpaceQualType to
> accept an optional SourceLocation, but that function is used in many
> places where a SourceLocation may not be available.
>

I think that's the wrong place to handle this. Instead, I would suggest you
look at the places where we form an AttributedType / AttributedTypeLoc,
which should be a lot rarer, and should always have a source location. Then
you can save the macro name information on the AttributedType itself (say,
as an IdentifierInfo*).

> On Mon, Aug 6, 2018 at 2:06 PM Richard Smith <richard at metafoo.co.uk>
> wrote:
> >
> > On Thu, 2 Aug 2018 at 20:53, John McCall via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
> >>
> >> > On Aug 2, 2018, at 7:49 PM, Leonard Chan <leonardchan at google.com>
> wrote:
> >> > As I have it now, I store a map of source locations to macro
> >> > definition names as they are being parsed and that's where I attempt
> >> > to find the attribute. Since the printing of the address_space is
> >> > controlled by the qualifier, I added an extra field to the qualifier
> >> > instead for storing the macro name. I can get this name during the
> >> > address space attribute construction where the source location of
> >> > address_space is available to me.
> >> >
> >> > Would you mind if I added you as a reviewer to this patch after I
> clean it up?
> >>
> >> Most of this preprocessor stuff is really beyond me; I'm probably not
> the best
> >> reviewer.  Richard might have a better suggestion.
> >>
> >> I *think* the macro name can be recovered from the existing recorded
> >> information in the AST; it's just not really obvious.  (Adding a method
> for this
> >> would be great!)  Again, I'm not an expert, but I think the idea is:
> >>   - You need to find the right level of macro expansion, keeping in
> mind that the
> >>     reference to the attribute macro might itself be written in a macro.
> >>   - You need to get the ExpansionInfo for that expansion.
> >>   - You need to verify that it's a macro body expansion and that it's
> not a function
> >>     macro expansion.
> >>   - At that point, getExpansionLocStart() should be the location of the
> token for the
> >>     macro, and you can just ask the preprocessor for the spelling of
> that token.
> >
> >
> > Yes, that's basically right. Lexer::getImmediateMacroName does most of
> this dance for you, but you'd still need to work out the right level of
> macro expansion for yourself. (You probably want to step out through
> expansion ranges until you reach one whose start is __attribute__ token and
> whose end is the closing `)` token, or something like that.) It would be
> nice to give this treatment to all cases where a macro exactly covers all
> the tokens in a type attribute.
> >
> > Depending on how you want this to work, there's another option:
> Preprocessor::getLastMacroWithSpelling can be used to find the macro
> defined most recently before a given source location that expands to a
> given sequence of tokens. Unlike getImmediateMacroName,
> getLastMacroWithSpelling should only be used when producing a diagnostic,
> because it needs to look through all defined macros (including all macros
> imported from modules) which is not really efficient enough to be
> appropriate for a compilation that doesn't produce diagnostics.
> >
> >>
> >> John.
> >>
> >>
> >> > On Thu, Aug 2, 2018 at 3:38 PM John McCall <rjmccall at apple.com>
> wrote:
> >> >>
> >> >>> On Aug 2, 2018, at 5:41 PM, Leonard Chan <leonardchan at google.com>
> wrote:
> >> >>>
> >> >>> On Thu, Aug 2, 2018 at 2:33 PM John McCall <rjmccall at apple.com>
> wrote:
> >> >>>>
> >> >>>>> On Aug 2, 2018, at 5:28 PM, Leonard Chan <leonardchan at google.com>
> wrote:
> >> >>>>>
> >> >>>>> Ok. So we will no longer use the pragma. We will instead opt for
> your
> >> >>>>> suggestion of printing the macro. So the new goals are now:
> >> >>>>>
> >> >>>>> - Having address_space() accept strings in addition to integers to
> >> >>>>> represent unique address spaces
> >> >>>>
> >> >>>> Is this still important if you get the diagnostic improvement?  If
> you were declaring address
> >> >>>> spaces somehow, this seems fine, but I don't really want to make
> address_space("whatever")
> >> >>>> implicitly declare an address space with specific semantics when
> it's not hard to imagine
> >> >>>> wanting that for other purposes in the future.
> >> >>>
> >> >>> Not entirely. This was something that's still remnant of when we
> were
> >> >>> using pragma. It's something that would be nice for us now, but
> could
> >> >>> also be discussed later down the line.
> >> >>>
> >> >>>>> - Changing the type printer to print the macro instead of the
> whole
> >> >>>>> attribute if the attribute was passed a string instead of an int.
> >> >>>>
> >> >>>> I don't know that the latter needs to be restricted; it seems like
> a nice usability improvement
> >> >>>> for all address-space users.
> >> >>>
> >> >>> Ok, so in general if address_space is used in a macro, then print
> the macro.
> >> >>
> >> >> Yeah.  You'll need to record this spelling information in the
> AttributedType, since
> >> >> the Type won't otherwise have a source location attached.
> >> >>
> >> >> I'm not sure we actually make an AttributedType for address_space,
> but doing
> >> >> so is reasonable.
> >> >>
> >> >> We should do a quick parse of the macro's token sequence to verify
> that it's just
> >> >> an attribute.  Unfortunately, I'm not really sure how to do that.
> >> >>
> >> >> John.
> >>
> >> _______________________________________________
> >> cfe-dev mailing list
> >> cfe-dev at lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20180807/c569992d/attachment.html>