Page MenuHomePhabricator

[clang][NFC] Remove dead code
ClosedPublic

Authored by wingo on Aug 19 2021, 3:06 AM.

Details

Summary

Remove code that has no effect in SemaType.cpp:processTypeAttrs.

Diff Detail

Event Timeline

wingo requested review of this revision.Aug 19 2021, 3:06 AM
wingo created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2021, 3:06 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Hi John, I am new to clang, having mostly worked on the WebAssembly target in llvm proper. I have a stack of patches related to address space treatment in codegen -- most of them NFC refactors to later allow the WebAssembly target to alloca in different address spaces. See https://lists.llvm.org/pipermail/cfe-dev/2021-July/068559.html for broader context. Would it be OK if I sent them your way? Cheers, Andy.

Hi, Andy. This patch is obviously okay, although it makes me wonder if it was introduced by a patch collision or something similar that needs closer attention. CC'ing Anastasia.

I'm not sure that I agree with your overall plan, though:

  • The WebAssembly operand stack is not a good match for an address space at the language level because it's not addressable at all. If you can't meaningfully have a pointer into the address space, then you don't really need this in the type system; it's more like a declaration modifier at best.
  • Allocating local variables on the operand stack ought to be a very straightforward analysis in the backend. There's not much optimization value in trying to do it in the frontend, and it's going to be problematic for things like coroutine lowering.
  • The security argument seems pretty weak, not because security isn't important but because this is not really an adequate basis for getting the tamper-proof guarantee you want. For example, LLVM passes can and do introduce its own allocas and store scalars into them sometimes. Really you need some sort of "tamper-proof" *type* which the compiler can make an end-to-end guarantee of non-tamper-ability for the values of, and while optimally this would be implemented by just keeping values on the operand stack, in the general case you will need to have some sort of strategy for keeping things in memory.
wingo added a subscriber: tlively.Aug 23 2021, 5:19 AM

Thanks for feedback! Following up on general question in D108464.

wingo added a comment.Aug 24 2021, 1:48 AM

@Anastasia is this good to go for you or does this need closer attention?

@Anastasia is this good to go for you or does this need closer attention?

Yeah I agree that this code seems like a left-over from some refactoring as it seems to have no effect.

Anastasia accepted this revision.Sep 1 2021, 11:21 AM

LGTM! Thanks!

This revision is now accepted and ready to land.Sep 1 2021, 11:21 AM
This revision was landed with ongoing or failed builds.Sep 20 2021, 3:16 AM
This revision was automatically updated to reflect the committed changes.