Add support for LLVM address spaces by peterohanley · Pull Request #148 · GaloisInc/llvm-pretty

peterohanley · 2024-11-26T18:59:48Z

Address spaces default to 0 and are currently mostly used by CHERI for capabilities (address space 200).

The thread_local property is properly read and reported.

Various formatting changes are implemented to more closely match llvm-dis output.

This affects many types and objects, and this work has only been tested by prettyprinting CHERI bc files.

This is required by llvm-as

peterohanley · 2024-11-26T19:12:31Z

src/Text/LLVM.hs


 ptrT :: Type -> Type
-ptrT  = PtrTo
+ptrT  = PtrTo (AddrSpace 0)


Hardcoded 0 address space here and elsewhere is reasonable because every previous use was with 0 (nothing else existed) but additional helpers may be useful later.

Could you factor AddrSpace 0 out into its own definition? Something like this (with an appropriate Haddock comment):

defaultAddrSpace :: AddrSpace defaultAddrSpace = Addr 0

And then use this in places where AddrSpace 0 is currently used. This would make it more obvious at a glance that AddrSpace 0 is a distinguished value, IMO.

RyanGlScott

I've done an initial pass (without having yet looked at the corresponding llvm-pretty-bc-parser changes).

RyanGlScott · 2024-11-26T19:16:00Z

src/Text/LLVM/AST.hs

+newtype AddrSpace = AddrSpace
+  { getAddrSpace :: Int
+  } deriving (Data, Eq, Generic, Ord, Show, Typeable) -- , Bits, FiniteBits)


Would you be willing to derive a Num instance for this? It would be convenient to be able to write, say, 0 instead of AddrSpace 0.

RyanGlScott · 2024-11-26T19:16:22Z

src/Text/LLVM/AST.hs

+newtype AddrSpace = AddrSpace
+  { getAddrSpace :: Int
+  } deriving (Data, Eq, Generic, Ord, Show, Typeable) -- , Bits, FiniteBits)


Can the commented-out code here be removed, or is there a need to keep it around?

RyanGlScott · 2024-11-26T19:26:54Z

src/Text/LLVM/AST.hs

+                | LocalExec
+    deriving (Data, Eq, Generic, Ord, Show, Typeable)
+
+-- ^ there is also None, use Nothing for it


What is this comment referring to?

RyanGlScott · 2024-11-26T19:43:37Z

src/Text/LLVM.hs


 ptrT :: Type -> Type
-ptrT  = PtrTo
+ptrT  = PtrTo (AddrSpace 0)


Could you factor AddrSpace 0 out into its own definition? Something like this (with an appropriate Haddock comment):

defaultAddrSpace :: AddrSpace defaultAddrSpace = Addr 0

And then use this in places where AddrSpace 0 is currently used. This would make it more obvious at a glance that AddrSpace 0 is a distinguished value, IMO.

RyanGlScott · 2024-11-26T19:44:19Z

src/Text/LLVM/AST.hs

  = BigEndian
  | LittleEndian
-  | PointerSize   !Int !Int !Int (Maybe Int) -- ^ address space, size, abi, pref
+  | PointerSize   !Bool !Int !Int !Int (Maybe Int) (Maybe Int) -- ^ fat pointer?, address space, size, abi, pref, "size of index used in GEP for address calculation"


Interestingly, the first Int here is intended to be an address space, but it is not using the AddrSpace type. I wonder if it should.

RyanGlScott · 2024-11-26T20:04:51Z

src/Text/LLVM/PP.hs

+        ppAddrSpace (gaAddrSpace ga) <+>
        constant


The comments above this ought to be updated, since it claims that only the value (and nothing else) is printed. Really, it ought to say that the address space and the value are printed now.

RyanGlScott · 2024-11-26T20:05:18Z

src/Text/LLVM/PP.hs

         <+> ppMds (defMetadata d)
         <+> char '{'
-         $+$ vcat (map ppBasicBlock (defBody d))
+         $+$ vcat (punctuate "" $ map ppBasicBlock (defBody d))


What is the motivation for this change?

RyanGlScott · 2024-11-26T20:05:36Z

src/Text/LLVM/PP.hs

-    DefaultVisibility   -> "default"
+    DefaultVisibility   -> empty


What is the motivation for this change?

RyanGlScott · 2024-11-26T20:06:39Z

src/Text/LLVM/PP.hs

    ConstUnaryArith op a   -> ppUnaryArithOp op <+> ppTyp' a
    ConstBit        op a b -> ppBitOp op   <+> ppTuple a b
-  where ppTuple  a b = parens $ ppTyped ppVal' a <> comma <+> ppVal' b
+  where ppTuple  a b = parens $ ppTyped ppVal' a <> comma <+> ppTyped ppVal' (const b <$> a)


What is the motivation for this change?

RyanGlScott · 2024-11-26T20:10:21Z

src/Text/LLVM/AST.hs

+                 | LocalUnnamedAddr
+    deriving (Data, Eq, Generic, Ord, Show, Typeable)
+
+newtype AddrSpace = AddrSpace


The comments in this part of the LLVM Language Reference Manual suggest that an address space's value must be less than 2^24. Is that worth putting in the Haddocks here?

kquick · 2024-11-26T21:24:11Z

src/Text/LLVM/AST.hs

 emptyGlobalAttrs  = GlobalAttrs
  { gaLinkage    = Nothing
  , gaVisibility = Nothing
+  , gaUnnamedAddr = Nothing


Missing gaThreadLocality = Nothing?

peterohanley-galois added 7 commits November 26, 2024 12:55

Add initial support for AddrSpace property

638b49b

This affects many types and objects, and this work has only been tested by prettyprinting CHERI bc files.

sort types compatibly with llvm-dis

9978019

Fix test for address spaces

cd1e17a

Properly print default alignment

fef33a4

print type on both sides of tuple

0631915

This is required by llvm-as

save thread_local property of globals

985932b

print function types matching llvm

f821469

peterohanley commented Nov 26, 2024

View reviewed changes

Revert part of formatting change

47d844f

RyanGlScott reviewed Nov 26, 2024

View reviewed changes

RyanGlScott mentioned this pull request Nov 26, 2024

Add support for LLVM address spaces GaloisInc/llvm-pretty-bc-parser#280

Open

kquick reviewed Nov 26, 2024

View reviewed changes

Conversation

peterohanley commented Nov 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanGlScott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants